Rails: Maintainable Code

Rails provides three ways to define a before save on an ActiveRecord::Base subclass.

# Version 1
class Transfer < ActiveRecord::Base
  before_save do |record|
    raise "Insufficient Funds" if record.original_account.balance - record.amount < 0
  end
end

# Version 2
class Transfer < ActiveRecord::Base
  before_save :raise_insufficient_funds_if_original_account_balance_minus_amount_is_less_than_zero
  
  def raise_insufficient_funds_if_original_account_balance_minus_amount_is_less_than_zero
    raise "Insufficient Funds" if original_account.balance - amount < 0
  end
end

# Version 3
class Transfer < ActiveRecord::Base
  def before_save
    raise "Insufficient Funds" if original_account.balance - amount < 0
  end
end

The most important thing about having three choices is selecting one per project. I've been on a project where the decision wasn't formalized and I once found a class that had both version 1 and 3. You can blame that on a bad programmer, but good programmers follow conventions.

Personally, I prefer version 1. I like the fact that the balance check is done in a block of code that is clearly associated with a before save.

I've worked with colleagues who prefer version 2. Their justification is that when they read the class they don't care about how the balance check is done, so they want it hidden away in another method instead of being directly in the block. First of all, I don't think reading a method name that describes how a method works is any better than reading the code itself. Furthermore, if your code isn't readable on it's own, you probably have a bigger problem.

The following example is what I would expect as a rebuttal from fans of version two.

# Version 4
class Transfer < ActiveRecord::Base
  before_save :check_for_sufficient_funds
  
  def check_for_sufficient_funds
    raise "Insufficient Funds" if original_account.balance - amount < 0
  end
end

Version 4 does a decent job of renaming the method to the intent instead of what it's doing. In isolation this looks like a decent choice, but as the class grows it becomes a maintenance issue in my experience. The first problem generally occurs when the class becomes long enough that the before_save declaration and the implementation aren't found on the same page. Yes, I can scroll, but I shouldn't need to. And, I can keep the implementation next to the declaration, but then I end up reading that method first. If I'm reading that method when I first open the class, why not just use version 1 anyway.

I expect Version 2 fans will next point out that my example is too trivial to see the benefits. This argument doesn't get very far with me. If the code is doing anything interesting it's probably because it's a business rule. Complex business rules shouldn't be defined in hooks. A hook should be something simple that makes your life easier or the logic should be delegated to a proper object for processing (topic covered by Evans in Domain Driven Design). So imagine that the logic for verifying sufficient funds is more complex, the following example is how it can be handled.

# Version 5
class Transfer < ActiveRecord::Base
  before_save do |record|
    SufficientFundsPolicy.validate(record)
  end
end

Breaking the logic out into a class is valuable for two reasons: Any complex logic is generally more testable when it lives in a PORO, and moving the logic to a class signals that the code is important and should be treated as such. Whether the code is in another class or not, deleting it should cause a failing test. That's not a good enough reason for leaving the complex logic in the Transfer class. Moving the code to another class will allow the SufficientFundsPolicy tests to break when the business rule is being incorrectly enforced, which should increase maintainability.

Version 3 is my least favorite. Mostly anyone who knows Rails will know that Version 3 is acceptable; however, Version 3 doesn't stand out when skimming a class that has several methods defined. Hook methods are methods with special behavior; however, aside from the name, Version 3 doesn't look special in anyway. A special method masquerading as a common method provides no benefit, but does run the risk of being mistaken for a common method.

Personally, I believe that Rails has the opportunity to be a more declarative framework which would result in more maintainable applications. There are already several class methods that allow for declarative coding: validates_*, before_*, after_*, etc. However, there are other actions that are special and masquerade as common. For example, controller actions are defined as common methods, but could as easily be defined with a class method.

# traditional
def new
  @menu_item = MenuItem.new
end

# declarative
action :new do
  @menu_item = MenuItem.new
end

Declarative code can provide both the implementation and the intent. Well written declarative code creates Expressive Software which is inherently more maintainable.