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?

Nice writeup, and echoes the sentiments I've read re: extracting mixins, before, as well. I will eventually get around to making a post about a recent experiment in an app I built that factors behavior into modules but doesn't hamper readability. The short version (for a Rails app) is that no behavior goes directly in the AR subclass, but goes in modules. However, the modules must relate to a specific behavior, are not allowed to assume the existence of any methods but the ones provided by their including class.

The moment one module needs to use another module's methods, it's screaming out to be a first class entity in the domain model, and so it becomes one. Might not work for all cases. After all, it was an experiment. But I liked how it made the "is this an object?" question/answer pop out very readily.

I'm gonna quote myself here: https://twitter.com/hakunin/statuses/113364211727482880. I completely agree that your initial example is bad. It's pretty much a method extracted from a class as-is, with all the dangling wires still connected to said class. If you extract things into a module, make sure it has a very narrow, well defined dependency on the target, or doesn't depend on it at all. (Like Enumerable only depends on #each).

Modules are often used in place of inheritance. This only reinforces my point - modules are supposed to be the originators of functionality, and not the users of it. So this again brings us to little or zero entanglement. They must make minimum to no assumptions about the target.

As far as DSLs go - I think in many cases their syntax is worth the indirection. It really does wonders for readability sometimes, and all you need is to find the context to clear things up.

I think modules

• should never have dependancies on other modules
• should never ever EVER muck with the internal state of its containing object
• should have as narrow an interface as possible with its containing object
• should have a coherent reason for existing that makes sense in isolation
• should be tested in isolation against a fake container

Mixins are a form of inheritance, and have all the same pitfalls, so its usually better to err on the side of composition over mixins. I also think that the rails style of usage where you gut a massive class, and organize its methods by moving them into modules is terrible every time. You are not addressing the problem that is probably trying to be solved (reducing complexity/responsability), AND you are introducing a new problem, which is that you break locality. Keeping a massive class in one file and adding comment "sections" isn't that great a solution, but it is better then moving them into mixins.

To me, the ideal mixin is Enumerable. It is a way to inject advanced iteration behavior into any object that implements primitive iteration behaviors (the each method). It is focused, powerful, easy to implement, useful, and never gets tangled in the logic of its containing class.

I like the class better, but it's relying a lot on what Order can do (feature envy?).

Perhaps:

class Purchase

  def initialize(amount, credit_card)
    @amount = amount
    @credit_card = credit_card
  end

  def submit
    result = Braintree::Transaction.sale(amount: @amount, credit_card: @credit_card)
    result.success?
  end

end

Then use something like:

class OrderController

def create
    @order = Order.create_from_cart(@cart)
    purchase = Purchase.new(@order.amount, @order.credit_card)

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

end

@ernie I look forward to hearing more about this solution.

@maxim, @mbriggs good points, especially about Enumerable being a good example of a module with a clear interface dependency.

@garethrees agreed, the Purchase class in my solution is doing a bit too much with Order.

Your final solution is pretty bad too. It suffers from a great deal of feature envy, with 3 references to Order in the submit method and, of course a dependency on order in the initialise method. All you have managed to do is flip the dependency around. If you change Order in similar ways to the ways your were complaining about for the old module, then you can break the new Purchase class.

@garethrees that's prolly worse. You're exposing order attributes in the Purchase initialize (which now takes a wider argument list than before), and the controller has just as much feature envy as the original Purchase class.

@MarkRatjens there are still issues with the last code example, but I think it is more than a simple dependency flip. The point I am making with this article is readability. I perfer the last code hands down because of that. There's not confusion as to what the current context is and who we're calling methods on.

I understand that it's always hard to make points with small examples and I agree with your observations about the dangers of modules. The key in this example is that you cannot take the purchase method (as given) away from Order, since it depends on order attributes so much.

The Purchasable module doesn't really justify itself, either, since there's only one method (hence the issue with tight, focussed examples). We don't really know what other methods are in order and which of these may have something to do purchasing. More of these may justify a module.

The solution to the example as given is put leave the purchase method in the Order class

I contend that readability is not improved since in either case something is hidden. I think the issue is whether you think procedural coupling by composition is more readable than an inclusion. It also depends on what you think needs to be co-located in one module. I am never very swayed by arguments that co-location includes readability because modularisation of any kind causes things to be moved out of your immediate view. Taken to the extreme, the co-location argument would require that all our software is in one file.

Yes, the implementation of the purchase method could be anywhere in the inheritance chain but should be easily found with a grep (if it isn't, then yes, you've taken modularisation too far). It's not really that hard.

I think there's always a fine line between creating a good abstraction vs creating confusing and over-complicated code. I also think the reason people tend to not extract methods into a new class is simply because lots of Rails programmers don't know how to do so in a dependable way. Let's face it, most people learn to program using Rails because they don't have to build abstraction layers in order to get a piece of code to work. Further, lots of folks learn much of what they know about Rails from (the AWESOME) Railscasts, etc. To my knowledge there aren't any episodes that cover building modules vs. abstracting into classes.

Back to the point, I think lots of Rails programmers would be willing to up their game, flex some Ruby muscle, and write cleaner code in the process...but they're hindered due to a lack of good tutorial content (at least, I haven't seen any) on how to dependably abstract into new classes that don't inherit from AR::Base.

For example, a great Railscast would be something like you did a while back when you moved methods from the controller to the model. But, this time, perhaps you cover abstracting model methods into new classes.

By the way, I tend to only use modules when I have to share code between models...doesn't happen too often, though.

Short answer: Your module example probably wants to be pulled out into a separate object that you use through composition. That way your interfaces and dependencies are more clear. Create an adapter class and use it within the order object.

@coghealth ... like any composition strategy applied in this case, how does it solve the feature envy?

@ryanb Thanks for this writeup. It is well-written and informative.

I have a doubt though:

  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.

Can you tell how @order cannot be of Order class even if it was defined in the initialize method?

@MarkRajens you can deal with feature envy, by doing something like this:

class Order
  def complete_purchase(purchase)
    purchase.complete(credit_card, amount)
  end

  def purchase_completed(transaction_reference)
    update_attibutes(:transaction_reference => transaction_reference)
  end

  def purchase_failed(error_message)
    # error handling
  end
end

class Purchase
  def initialize(order)
    @order = order
  end

  def submit
    order.complete_purchase(self)
  end

  def complete(credit_card, amount)
    result = Braintree::Transaction.sale(amount: @amount, credit_card: @credit_card)
    if result.success?
      order.purchase_succeeded(result.transaction_reference)
    else
      order.purchase_failed(result.error)
    end
  end
end

In this case the Purchase and Order communicate entirely through a defined protocol, and they no nothing of each other's internals.

It adds indirection, and perhaps makes the code harder to follow, but it allows you to vary both Order and Purchase independently.

In this instance we might decide its a step too far. While we are using the state of Order within Purchase we are not making decisions based on that state. As always that's down to taste, and what you need the code to do.

Interestingly though, consider this class:

class Sponsorship
  def complete_purchase(purchase)
    purchase.complete(card, total_amount)
  end

  def purchase_completed(transaction_reference)
    # ...
  end

  def purchase_failed(error_message)
    # ...
  end

  def total_amount
    mile_run * per_mile_amount
  end
end

Sorry previous comment to: @MarkRatjens

Modules encouraging the designer to raise the level of abstraction of the design to zero in on the absolutely essential/minimal messaging necessary to integrate with the rest of the system... sounds like modules doing their job. :)

@MarkRatjens - How would you express the code?

@MarkRatjens

All you have managed to do is flip the dependency around.

Precisely! Dependency inversion is very much desired here. That we can duck-type the "order" (which to your point, and @ryanb's comments, could use for a more explicit interface) lowers the dependencies between these objects as they no longer need to know the contant names. This means Purchase knows less about the context in which it is used, and thus can focus just on its job. A happy side effect is that Purchase could also be plugged in elsewhere to work with other "order-like" things.

The Purchasable module doesn't really justify itself, either, since there's only one method...

Are you arguing that LOC is a measure of the "worth" of a bit of code? Does cohesion not matter? Nor the amount of coupling?

I am never very swayed by arguments that co-location includes readability because modularisation of any kind causes things to be moved out of your immediate view. Taken to the extreme, the co-location argument would require that all our software is in one file.

There is a difference between lexical locality and cognitive locality. We are shooting for a very high cognitive locality, meaning that we can keep a short mental stack, only needing to think about/remember the things directly related to our current work site.

the implementation of the purchase method could be anywhere in the inheritance chain but should be easily found with a grep

Please see GDD - Grep-driven Development. tldr; this is not a desirable 'feature' of a system.

@ryanb : Thanks so much for the well thought out post. I've come to lump many of the patterns you're describing here under a single term: BOMM - Bag of Methods Module. It is great to see a more responsible, less snarky, write-up on the matter.

@ryanb, you should checkout my activerecord-concernable gem. This adds a simple DSL to ActiveRecord objects for defining concerns that I believe clears up most of the issues you describe here (Lack of context, Unclear Interface Dependency, Too Many Modules).

The one thing it doesn't necessarily address is giving you a complete picture of your model in a single place, but this can be done with specs if desired. Of course, as an application grows you really don't want to be required to understand the entire thing in order to make changes effectively. This DSL helps establish and expose an underlying design within the models that allows for a clearer understanding about the individual components in isolation. It's pretty much concerns with some conventions, less code, and less indirection (Just for clarity, this library doesn't reinvent any of ActiveSupport::Concern, but instead builds upon it).

I think you'll find this solution elegant for defining light weight tie-in functionality (a prevalent need), while any heavy lifting can still be pulled out into a specialized class where and as needed. Hope you like.

As always, thanks for posting your thoughts, it's provided great insight and inspiration.

Back to the point, I think lots of Rails programmers would be willing to up their game, flex some Ruby muscle, and write cleaner code in the process...but they're hindered due to a lack of good tutorial content (at least, I haven't seen any) on how to dependably abstract into new classes that don't inherit from AR::Base.

YES! After a couple years of awful ruby and rails practices, I've been cleaning up my game: learning git inside and out, writing the damn tests, working to refactor, sound deployments, and it goes on like this..

Clearly this is an important subject - I read articles from very established rubyists who balk at how beginner to intermediate rails users rely on Active Record as a crutch and need to use Pure Ruby Classes: but I really can't find a complete tutorial. So I will figure it out - but many others might not have the drive I finally developed.

Thanks all.

Using concerns is a horrible design in any Rails application. Not all ideas that comes out of the Rails core team is good. This is just one of the worst ideas from them.(Coffeescript is another ). This mainly happens when the developer does not know anything about domain objects. Some of the folks here have done the same mistake of mixing business logic with persistence. Read the Single Responsibility Principle. I think of it as Single Purpose Principle, since Responsibility actually has the different and correct meaning in CRC. Why does the domain object need to know or be aware of persistence? It is irrelevant.

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.