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:
- What does a given method do?
- What is the current object context that I am in?
- 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.
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.
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.
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.
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:
- What does a given method do? It's easy enough to open the Order class and find out.
- What is the current object context that I am in? A Purchase instance
- 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?
@ryanb Thanks for this writeup. It is well-written and informative.
I have a doubt though:
Can you tell how
@order
cannot be of Order class even if it was defined in theinitialize
method?