venkatch789 · September 29, 2015 07:28 · Nov 29, 2012 · Nov 29, 2012 · Nov 29, 2012
diff --git a/issues_with_modules.md b/issues_with_modules.md
@@ -88,7 +88,7 @@ Modules can be great at adding behavior when they don't have tight dependencies
 module Purchasable
   def purchase(total, card)
     result = Braintree::Transaction.sale(amount: total, credit_card: card)
-    yield result.success? # leave it up to the caller to mark as purchased
+    result.success? # leave it up to the caller to mark as purchased
   end
 end
 ```
@@ -111,7 +111,7 @@ end
 
 Here whenever I see a method called on `order` it is a guessing game trying to determine where it is defined. True `order.purchase` is easy enough, but not all methods correlate so nicely.
 
-Another issue is conflicting methods. There's a shared namespace across all modules. It's only a matter of time before they collide. Sometimes this is used intentionally to override behavior. The [ActiveRecord::Validations](https://github.com/rails/rails/blob/master/activerecord/lib/active_record/validations.rb#L28) module is a classic example of this. It overrides various `save` methods to add validations.
+Another issue is conflicting methods. There's a shared namespace across all included modules. It's only a matter of time before they collide. Sometimes this is used intentionally to override behavior. The [ActiveRecord::Validations](https://github.com/rails/rails/blob/master/activerecord/lib/active_record/validations.rb#L28) module is a classic example of this. It overrides various `save` methods to add validations.
 
 What's the problem with this? It makes it difficult to have a clear picture of what a method call is doing. Let's say you want to know what Rails is doing when calling `order.save` so you take a look at the [method definition](https://github.com/rails/rails/blob/master/activerecord/lib/active_record/persistence.rb#L100). This isn't telling you the whole story since another module overrides behavior. We also have a dependency on the order modules are included. Ick.
 

diff --git a/issues_with_modules.md b/issues_with_modules.md
@@ -64,7 +64,7 @@ end
 There are many other issues with this approach (it tries to hide the complexity of the class), but at least the context of the code is clearer.
 
 
-## Lack of Interface Dependency
+## Unclear Interface Dependency
 
 Now what if you have a module intended to be used in multiple classes? Let's take another look at that Purchasable module:
 
@@ -115,7 +115,7 @@ Another issue is conflicting methods. There's a shared namespace across all modu
 
 What's the problem with this? It makes it difficult to have a clear picture of what a method call is doing. Let's say you want to know what Rails is doing when calling `order.save` so you take a look at the [method definition](https://github.com/rails/rails/blob/master/activerecord/lib/active_record/persistence.rb#L100). This isn't telling you the whole story since another module overrides behavior. We also have a dependency on the order modules are included. Ick.
 
-Many of these arguments can apply to inheritance, but there the chain is normally not very long. Think of it this way, every time you include a module it's adding to the inheritance chain.
+Many of these arguments can apply to inheritance, but there the chain is normally not as long. Think of it this way, every time you include a module it's adding to the inheritance chain.
 
 
 ## Try a Class

diff --git a/issues_with_modules.md b/issues_with_modules.md
@@ -0,0 +1,152 @@
+# My issues with Modules
+
+In researching topics for RailsCasts I often read code in Rails and other gems. This is a great exercise to do. Not only will you pick up some coding tips, but it can help you better understand what makes code readable.
+
+A common practice to organize code in gems is to divide it into modules. When this is done extensively I find it becomes very difficult to read. Before I explain further, a quick detour on `instance_eval`.
+
+You can find `instance_eval` used in many DSLs: from routes to state machines. Here's an example from Thinking Sphinx.
+
+```ruby
+class Article < ActiveRecord::Base
+  define_index do
+    indexes subject, sortable: true
+    indexes content
+    indexes author.name, as: :author, sortable: true
+
+    has author_id, created_at, updated_at
+  end
+end
+```
+
+When working through a snippet of code I like to ask myself the following questions:
+
+1. What does a given method do?
+2. What is the current object context that I am in?
+3. What other methods can I call here?
+
+These quesions are difficult to answer in that `define_index` block because `instance_eval` is used under the hood to swap out the current context.
+
+I'm not picking on Thinking Sphinx, I think this is an acceptable use of `instance_eval` since it's done in a very deliberate and limited fashion for the sake of a DSL. There's also a decent amount of external documentation to help answer these questions.
+
+Now let's turn our attention to modules.
+
+
+## Lack of Context
+
+Let's say you are browsing through an unfamiliar code base and stumble across this module.
+
+```ruby
+module Purchasable
+  def purchase
+    result = Braintree::Transaction.sale(amount: total, credit_card: card)
+    if result.success?
+      self.purchased_at = Time.zone.now
+      save!
+    end
+  end
+end
+```
+
+Ask yourself the same questions as above. How do I find out what a called method does? In what object context am I in? We are left in the same state of confusion as with `instance_eval`, but this isn't for the sake of a DSL. This technique is spread all throughout the code base as an attempt to organize it.
+
+Looking at that module, I would guess it's meant to be included on some kind of Order model, but that is an assumption since there is no mention of Order anywhere here.
+
+If the primary goal of the module is to organize a large class, consider namespacing it with that class.
+
+```ruby
+class Order
+  module Purchasable
+    # ...
+  end
+end
+```
+
+There are many other issues with this approach (it tries to hide the complexity of the class), but at least the context of the code is clearer.
+
+
+## Lack of Interface Dependency
+
+Now what if you have a module intended to be used in multiple classes? Let's take another look at that Purchasable module:
+
+```ruby
+module Purchasable
+  def purchase
+    result = Braintree::Transaction.sale(amount: total, credit_card: card)
+    if result.success?
+      self.purchased_at = Time.zone.now
+      save!
+    end
+  end
+end
+```
+
+For this to be used elsewhere, the class must respond to the same interface: `total`, `card`, `purchased_at=` and `save!`. If we change the behavior it is very easy to call another method on Order and suddenly we've broken the other class that includes this module. The required interface isn't clearly defined and easy to change on a whim.
+
+Modules can be great at adding behavior when they don't have tight dependencies on the class that's including it. For example:
+
+```ruby
+module Purchasable
+  def purchase(total, card)
+    result = Braintree::Transaction.sale(amount: total, credit_card: card)
+    yield result.success? # leave it up to the caller to mark as purchased
+  end
+end
+```
+
+This could be easily shared and included in other classes.
+
+
+## Too Many Modules
+
+Let's take a look at the other side of the coin, the class that's including modules.
+
+```ruby
+class Order < ActiveRecord::Base
+  include Purchasable
+  include Shippable
+  include Searchable
+  # a dozen other modules here...
+end
+```
+
+Here whenever I see a method called on `order` it is a guessing game trying to determine where it is defined. True `order.purchase` is easy enough, but not all methods correlate so nicely.
+
+Another issue is conflicting methods. There's a shared namespace across all modules. It's only a matter of time before they collide. Sometimes this is used intentionally to override behavior. The [ActiveRecord::Validations](https://github.com/rails/rails/blob/master/activerecord/lib/active_record/validations.rb#L28) module is a classic example of this. It overrides various `save` methods to add validations.
+
+What's the problem with this? It makes it difficult to have a clear picture of what a method call is doing. Let's say you want to know what Rails is doing when calling `order.save` so you take a look at the [method definition](https://github.com/rails/rails/blob/master/activerecord/lib/active_record/persistence.rb#L100). This isn't telling you the whole story since another module overrides behavior. We also have a dependency on the order modules are included. Ick.
+
+Many of these arguments can apply to inheritance, but there the chain is normally not very long. Think of it this way, every time you include a module it's adding to the inheritance chain.
+
+
+## Try a Class
+
+Instead of hiding complexity with modules, consider creating a class.
+
+```ruby
+class Purchase
+  def initialize(order)
+    @order = order
+  end
+
+  def submit
+    result = Braintree::Transaction.sale(amount: @order.total, credit_card: @order.card)
+    if result.success?
+      @order.mark_as_purchased!
+    end
+  end
+end
+```
+
+Now ask the questions again:
+
+1. What does a given method do? *It's easy enough to open the Order class and find out.*
+2. What is the current object context that I am in? *A Purchase instance*
+3. What other methods can I call here? *Anything defined on Purchase*
+
+Some of the previous issues are still present here. Since Ruby is dynamically typed there's no saying that `@order` is an `Order` class, but the naming makes it clear what the intent is.
+
+If we wanted to support different types of orders, I would probably make the interface stricter (maybe not work on Order directly).
+
+Overall, I think modules have their uses, but please consider using classes next time you're faced with refactoring complexity.
+
+What are you thoughts?