Written December 24, 2012. Tagged Object design.
Mixing responsibilities in the same class discourages refactoring, much like class methods do.
Say you have a Receipt
that you need to check for fraud.
You could just add another method:
class Receipt < ActiveRecord::Base
# …
def fraudulent?
# …
end
end
If Receipt#fraudulent?
becomes complex enough, you may want to refactor by extracting private methods:
class Receipt < ActiveRecord::Base
validate :validate_something
def fraudulent?
check_this
check_that
end
private
def validate_something
# …
end
def check_this
# …
end
def check_that
# …
end
end
But this quickly becomes a mess.
The class can end up with a high number of unrelated private methods, making the class harder to understand and making bugs and dead code more likely. Perhaps you start to organize with clumsy names like check_this_for_fraud
, or with comment headers, or you avoid extracting them at all.
You may even accidentally break unrelated Receipt
code while working on the fraud detection. You will definitely see code unrelated to fraud when that's all you need to focus on, and that may lead you to load more context into your head than you need for the task at hand.
You can extract the responsibility to a class of its own:
class Receipt
class FraudChecker
def initialize(receipt)
@receipt = receipt
end
def fraudulent?
check_this
check_that
end
private
def check_this
# …
end
def check_that
# …
end
end
end
Since this class is only concerned with receipts, I would probably call it Receipt::FraudChecker
, consider it an internal detail of Receipt
, and delegate to it:
class Receipt < ActiveRecord::Base
def fraudulent?
FraudChecker.new(self).fraudulent?
end
end
This means code outside Receipt
has less things to care about – the single method Receipt#fraudulent?
instead of the entire class Receipt::FraudChecker
and whatever it may contain.
Alternatively, we could remove this delegation and make Receipt
totally unaware of FraudChecker
. That would give some benefits. The dependency would only go in one direction, so changes to FraudChecker
wouldn't necessitate changes in Receipt
. But as Receipt
only knows a minimum about FraudChecker
, and we do get good value from that knowledge, I wouldn't break them apart any further.
Oh, and the fraud checking is no longer in a subclass of ActiveRecord::Base
, so it's easier to test without Rails if that's what you're into.
You could also use a module for this:
class Receipt < ActiveRecord::Base
include FraudCheckable
end
module Receipt::FraudCheckable
def fraudulent?
check_this
check_that
end
private
def check_this
# …
end
def check_that
# …
end
end
This has many of the same benefits. However, there is some risk of method collisions. Even if they probably won't collide, you would need to keep the entire class in your head (or your editor) to make sure. The responsibility of checking for fraud will be a little muddied by the fact that the code runs in the context of a full Receipt
instance.
Those are admittedly not very strong arguments against modules. But since extracting classes is about the same amount of ceremony, I'd go with that.
I've found extracting classes this way to be very low cost and high yield. It's very little extra code and takes very little extra time. It's very little extra indirection and not hard to follow.
As with any other technique, it's not always the answer. It might not be worth it for a method that's just a few lines of code. The more code, the more obviously it's a responsibility worth separating.
The important thing is to be aware that mixed responsibilities resist refactoring, so you consider extraction in situations like the above.
For further reading, see: single responsibility principle (SRP), the Extract Class refactoring, the Replace Method with Method Object refactoring.
This post was inspired by discussion on the Ruby Rogues Parley mailing list. I can really recommend it if you like discussing higher-level subjects like object design, testing strategies and music for hackers.