Saturday, October 06, 2007

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.

14 comments:

  1. Personally I'm a fan of version 2, albeit where the method name describes the intent rather than the implementation. That way I can hide the (private) implementation further down the class and the first page of the model class is just a set of declarations of intent.

    Then again, chances are I'd write:

    validates_sufficient_funds :in => :source_account

    or something along those lines.

    ReplyDelete
  2. i love the analysis here, and that last little bit (action :new do) is quite similar to something i've cooked up recently, and i think we're onto the same idea. just some props, this is one of my favorite programmer blogs and posts like these are the reason why. :)

    ReplyDelete
  3. I think the big win for version #2 is that it can be documented (rdoc).

    Plus, I don't see the advantage in breaking a single method out into a module as far as moderately complex business rules go.

    Of course that's a great way to go if you need a whole graph of objects/methods to represent that rule.

    ReplyDelete
  4. I think version 4 is by far the best way for a simple example like this. You say you shouldn't have to scroll to look at the code, but I say you shouldn't have to look at the code in the first place! Put another way, I'm sure it doesn't bother you to use validates_presence_of and not see the source code embedded in this class.

    ReplyDelete
  5. The reality is that you do end up looking at the code. But, with version 4 you end up looking at it while it masquerades as a common method. You can ignore the code inside Version 1 as easily as you ignore the before_save class method call with a symbol, the difference is that you don't end up finding some other method later when you are missing it's context.

    Also, I didn't write the implementation of validates_presence_of, so I don't think it's a good example.

    ReplyDelete
  6. I think version 4's big win is that it reveals intent. You could take out the implementation and show it to a customer and verify that it does what you want to. Certainly there's not a whole lot of difference between verbalizing "before saving, we check for sufficient funds" and "before saving, we check that the transfer amount is less than the balance." However version #4 maps directly on to how you'd communicate it, and I think that's important for developing a ubiquitous language.

    An example of this from Domain-Driven Design is on pp220-221. Evans starts off with simple bucket class (apparently I can't use <code> or <pre>)

    It's pretty obvious what it does, of course, but we can extract that conditional to an intention-revealing method

    I see this as pretty much the same situation. I have a feeling that you and I probably just have different styles in this regard. You say you shouldn't need to scroll down to see the implementation. I would prefer to have the implementation tucked away behind an intention-revealing name so that I don't have to mentally parse the implementation every time I look at it. I can just read the name and know what's supposed to happen. I'm happy to scroll down to the implementation when I need to dig into it, but for the most part I don't want to even know it's there.

    As far as it masquerading as a common method, I see where you're coming from. I think this is where method visibility can help out. Make the check_for_sufficient_funds method private, and it's obvious that it's used only internally to the class.

    I guess my main point is that version #1 feels so incongruent with your desire to make Rails more declarative. The validation you perform is simple, but you're still showing the details of how it does it, instead of declaring what it should do and defining the how elsewhere. To me, declarative programming is all about being able to look at code and understand what it does without having to understand the how. If you need to understand or change the how, then you can go look up the implementation.

    ReplyDelete
  7. When you say you shouldn't use business logic in your hooks are you just saying that you shouldn't have validations in your hooks?

    Seems like there are some valid cases where you do want business logic in your hooks but I understand that you wouldn't want it adding errors to an attribute or to base.

    Is that at all correct or am I just missing something?

    ReplyDelete
  8. Your argument for version #2 being a bad choice due to the fact that you have to scroll down to see the implementation seems like a bad one. By moving the logic into another class as in version #4 you now have to open an another file to see the implementation. After reading Pat's comments though I think I have a better understanding of the advantages of version #4. (Domain Driven Design is still on my to read list.) In a way though, I think both version #2 and version #4 are reaching for the same goal. They both provide a way of communicating the intent of the code. You make some very good points about maintainability and forming a more direct language with version #4, so maybe you have won me over. I guess it is hard for me to think that such a simple rule warrants an entire separate class when an intention-revealing method will accomplish the same thing, IMO. Of course, every rule starts out as 'simple' and I could see more logic being required later on that would definitely warrant the existent of a SufficientFundsPolicy . Perhaps version #2 is suited for simple cases and version #4 could be used for refactoring when more logic needs to be added? Of course then we go back to the need to being consistent with the version we use.... Anyways, thanks for the post, you got me thinking.

    BTW, have you looked at the make_resourceful plugin? It is a step towards having a declarative language for controllers just like we do on certain aspects of models in AR.

    ReplyDelete
  9. Chuck, I'd say a business rule is something that should stand out in some way. Putting it in a hook can make it hard to find and appear unimportant. For example "is that guard clause there for legacy reasons or is it a business rule?"

    ReplyDelete
  10. Ben, I haven't thanks for the tip, I'll check it out.

    ReplyDelete
  11. Pat, Context is king. Reading your comment it appears that you are worried about writing the code so that you can easily maintain it. That may be a perfectly valid way if you plan on running your own business and maintaining the software.

    For me, life is quite different. The code I write today is more likely to been worked on tomorrow by someone other than me. And, I almost always have to work on code that someone else has written. Finding a style that takes little explaining or searching is one that greatly benefits me. It just so happens that I also believe that style will help almost anyone whos working on a team of more than 3 developers.

    You may benefit from hiding the method from yourself, but that also means that it's hidden from everyone else who follows you in the codebase.

    ReplyDelete
  12. This comment has been removed by the author.

    ReplyDelete
  13. 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 think this is a specific instance of a more general problem, which is that Rails controllers get unwieldy pretty quickly when you're dealing with complex apps. Mapping controller actions to method names works well in the simple case, but when you also need methods within the controller, you end up with methods which could be actions, or not, maybe. The clarity is scoped to small apps, and breaks down with more complicated code.

    I'm not into Version 3 at all, but I actually prefer Version 2 to Version 1. Koz from Rails Core blogged something similar on The Rails Way a few days ago, where he broke out validation logic into methods and hooked those methods into validate :method, equivalent to before_save :method. He intentionally made it more verbose in order to make it more obvious.

    That being said, I think the PORO point is a good point. The plain old Ruby object is woefully underused in Rails development. My presenter objects are just POROs. It's easier to test them and it's often all you need.

    ReplyDelete
  14. Vince8:21 PM

    I like #4. before_save is part of the event model. I don't write domain logic in the event model, I call domain logic from the event model. I keep my events skinny, just like controllers, which are the model's public event model.

    Now, if we need to, we can check for sufficient funds earlier in the life cycle, perhaps as the customer is filling their shopping cart. Or we might have more things to check before saving the transaction.

    ReplyDelete

Note: Only a member of this blog may post a comment.