Points on how modules can make code difficult to read.

issues_with_modules.md

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.

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.

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.

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.

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:

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:

module Purchasable
  def purchase(total, card)
    result = Braintree::Transaction.sale(amount: total, credit_card: card)
    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.

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 included modules. It's only a matter of time before they collide. Sometimes this is used intentionally to override behavior. The ActiveRecord::Validations 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. 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 as 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.

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?

I might be DCI/SimpleDelegator-happy ,but my solution is similar to that of @garethrees except it is like

class Purchase < SimpleDelegator
  def submit
    result = Braintree::Transaction.sale(amount: amount, credit_card: credit_card)
    result.success?
  end
end

with

class OrderController
  def create
    @order = Order.create_from_cart(@cart)
    purchase = Purchase.new(@order)

    if purchase.submit
      @order.mark_as_purchased!
    else
      # handle errors
    end
  end
end

I believe this gets out of feature-envy, observes SRP, etc. at the risk of schizophrenia.