Many Skinny Methods
This refactoring is based on a topic Marcel and I covered at RailsConf Europe.
Before
1 2 3 4 5 6 7 8 9 10 11 12 |
class Expense < ActiveRecord::Base belongs_to :payee protected # Nice and concise, but what happens as we add more rules # and how do we write test cases for the four different possible # validation states? def validate errors.add("Not enough funds") if payee.balance - amount > 0 errors.add("Charge is too great") if payee.account.maximum_allowable_charge > amount end end |
After
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 |
class Expense < ActiveRecord::Base belongs_to :payee # Instead of one large validation method, break each individual # rule into methods, and declare them here. validate :ensure_balance_is_sufficient_to_cover_amount validate :amount_does_not_exceed_maximum_allowable_charge protected # These validation callbacks simply add error messages if a particular # condition is met. Each of them can be tested and understood on their own # without having to understand the entire body of the validate method. def ensure_balance_is_sufficient_to_cover_amount errors.add("Not enough funds") if insufficient_funds? end def amount_does_not_exceed_maximum_allowable_charge errors.add("Charge is too great") if exceeds_maximum_allowable_charge? end # By defining separate predicate methods we can test each of them individually # and new programmers can see the intent of the code, not just the implementation. # Instead of subtracting the amount from the balance and checking if the # value is greater than 0, change the implementation to mirror the intent. # There was a bug in the before code, this is more obvious. def insufficient_funds? amount > payee.balance end def exceeds_maximum_allowable_charge? payee.account.maximum_allowable_charge > amount end end |
While the refactored version may have more lines of code, but don’t let that scare you. It’s far more important for code to be human readable than incredibly concise.
This kind of refactoring (making code bigger, but more expressive) is a skill I have yet to master.
But maybe the two last helper methods should be declared public? They certainly can be re-used.
Nice refactoring. It’s very easy, with Ruby, to get too concise as you say. Let’s not forget that one of Ruby’s major advantages is the ability to express things in sentences.
I like the ‘validate :method_to_validate_with’. I didn’t realise you could do this. It keeps things consistent with the other validations that are declared this way.
@Mislav – Personally I don’t make things public unless I need to (unless you’re writting an API). It makes people think ‘is there a higher level call I can make instead’ before they go ahead and make it public and it helps keep the interface clean. Remember YAGNI.
And how exactly would this affect the way the four different possible validation states? I must admit that the code is more readable, but since it doesn’t change any of the business logic, the tests keep the same, right?
Really nice and beautiful example. I totally agree on your statement that code should be as human readable as possible.
For those looking at refactoring, I suggest picking up Martin Fowler’s book on it, it’s got a tonne of detail about how and where to refactor, along with worked examples for many different refactoring techniques.
I really don’t find this refactoring very appealing – it seems to have introduced a huge amount of duplication – e.g. the intent “make sure there’s enough money” is repeated in four places: the validation method name, the validation statement, the text string and the boolean check function. Is this really necessary?
Perhaps if there was a tangled mess of validation logic to begin with I could see the benefit, but otherwise it seems like it’s heading towards http://developers.slashdot.org/comments.pl?sid=33602&cid=3636102 ;)
1. more code makes reading code more difficult, not more easy 2. more code makes runtime slower.
It’s especially true when you introduce a huge heap of meta-programming magic by using “validate” which probably uses even more magic like alias_method_chain. Reading, debugging, and actually running this new version has all the minuses, none of the pluses.
This is not a good enough reason, especially when people easily disagree with it: “It’s far more important for code to be human readable than incredibly concise.”
just my 2cc
I agree with Hadley. While there are only two validations in the `validate’ method, it’s still possible to test each case or a combination of them in a clean and simple way: instantiate a record, make it erroneous, and check the error messages that must have stacked up. Only decompose the `validate’ method when this process becomes unmaintainable.
So, to me, this refactoring was useless except for the `amount – payee.balance > 0’ which becomes `amount > payee.balance’. With that change, I find the `validate’ method more human readable than the refactored version (very concise and each line can be read as a sentence).
This would seem like a happier medium to me:
class Expense < ActiveRecord::Base belongs_to :payee
Personally, I find that pretty humanly readable.
end
Rails is an opinionated framework and I think we should go ahead a deprecate the usage of overriding the validate method (used in the Before).
def exceeds_maximum_allowable_charge? payee.account.maximum_allowable_charge > amount endbedef exceeds_maximum_allowable_charge? amount > payee.account.maximum_allowable_charge end?I like this resulting code, but just another thought.. validations can be a bit like “tests” (or specs, if you’re that way inclined), right? I wonder if there’s a way to change the syntax a bit to make validation look more like a “live” test suite (or set of specs) so that the familiarity of syntax is there in both cases..
If I were reading someone else’s code (and in this case I am), I’d much prefer the after. It has all the validations consolidated for easy reading, telling me “what” the validations are doing without me having to figure out the “how”.
Just like reading an rdoc, most of the time I don’t care “how” each method is written, just “what” it does.
I agree with the comment about duplication. And I would also say that I find
to be more readable than
The former has the benefit of never getting out of sync with itself, unlike a method’s code and its name.
///ark
I agree, it’s easier to read. Maintainability is up. But what when you end up with 500 LOC in your models ?
And you should try using http://refactormycode.com/ for pasting your code or just for getting feedbacks on refactorings.
I like this refactoring.
Extracting logic into functions should be about more than cutting down code. It’s breaking logic into discrete facts. With “insufficient_funds?”, it’s easier to support buying items on credit.
However, I think that method should be public.
@Matthijs: There are still four permutations, but you can test them individually and when your test fails it will be something like test_insufficient_funds rather than test_validate. A small difference, but one that matters.
@Phil: Yeah, validate is not documented, what’s up with that? :P
@Hadley: As with any of our examples, we’ve had to cut down the context a little. But imagine this was a fully featured application with 10-15 validations, a bunch of associations and some business logic methods. If you were then tasked with tracking down a bug in a 30 line validate method you’d prefer the ‘after’. But with one or two, it’s certainly debatable.
@Evgeny: I appreciate your two cents but respectfully disagree. For starters validate doesn’t use alias_method_chain at all, and I certainly wouldn’t opt for an ugly implementation until I had profiling reports showing there was a problem.
Given that in almost all actions you’re only going to be validating a single instance, you’re talking about four method calls, that will not matter in the scheme of things. Premature optimisation causes far more problems than intention revealing code.
@Luke: Well caught!
@Peter Cooper: I don’t quite follow you there, but there’s definitely some similarities going on. Validations are, to an extent, invariants you express about your models. Design by contract etc could be an interesting approach to use.
Well, I agree completely with “It’s far more important for code to be human readable than incredibly concise”.
In my experience, code is written for business purposes, paid for by a business, businesses change, and so cost-effective maintainability is very important, even more so than pure speed in many cases.
Developers hired to maintain code are often very different people to those who created it in the first place and have moved on to the next thing. Different skillsets, experience, approach, ability, confidence. They need all the help you can give them.
Any part of the code that expresses business logic should be (almost) readable by a business person.
Framework code, well, that’s a different story. Go for efficiency.
Koz,
I agree with where you guys are going here but I don’t like the flipping back and forth between method-name-intention.
ensure_balance_is_sufficient_to_cover_amount returns true if insufficient_funds? returns false.
It’s a bit like: “I need to decide whether to allow this record to be saved. Say ‘yes’ if you want me to say ‘no’.”
I would have done ensure_sufficient_funds checks sufficient_funds?
But then you have to ask, do we really need ensure_xxx just to add an error string?
validate :sufficient_funds?, :message => “Not enough funds” validate :within_maximum_allowable_charge?, :message => “Charge is too great”
To me it’s much more declarative. Too bad it doesn’t work :-)
Trevor
grumble… formatting messed up. Here is what I meant to say:
validate :sufficient_funds?, :message => “Not enough funds”
validate :within_maximum_allowable_charge?, :message => “Charge is too great”
Trevor
@Trevor: Now that’s an interesting idea for a plugin / patch…
Okay then…
How about: http://pastie.caboo.se/private/gyka9yxrfblczytfkekq
I’m not a fan of: Does not satisfy: “Within maximum allowable charge?” but it’s better than a kick in the pants if you don’t specify a :message.
Patchworthy?
Trev
It appears we now know where Koz sits in the testing private/protected methods religious argument. I’m not really sure where I’m at with the thing…
Why is there so much emphasis on ‘human readable’? Ruby is already extremely ‘human readable’ and if your decent with Ruby, you can easily read any written code with no problem, IMO.
@Barry: Well, actually I’d make those predicates public if I was going to test them. But I don’t tend to take a position on religious arguments :)
@Koz: Or would you only test those predicates if they were public. :)
I like the first and the second code. The refactoring was OK. This may not be the case, but I think that is somethin worng on the fact that some people spend hours trying to make their code more clean and more pretty. They do not add something useful, just re-code something that was just OK. The first code is OK! That means no need to apply some refactoring on it. I just can´t understand how a programmer looses time to refactor something that is already OK. We should care about FUNCTIONALITY and good code and tested code. Change a good code to just make it prettier and don´t add functionality is a loss of time. The fisrt code is OK!!!
@Mario
I’m sure a lot of (clueless) business owners would agree with you. However, another argument for refactoring is that unless you do, it will become more difficult to add new functionality as time progresses and your code base gets larger with code that is simply “OK.”
It’s not just about being pretty or readable, it’s about being able to expand and add. Chances are good when you were writing line 1 you had no idea what line 5,000 would look like.
@Mario
It’s not just the benefit of being able to add new functionality over time, its being able to come back, in six months to a piece of code and not have to spend a huge amount of time retracing steps to figure out what is happening or what is broken.
As Koz said, this particular example is not big enough to really display the impact. You can figure out what it is doing very quickly in either example. But if it were much longer, with a lot more code and all you wanted was to double check what validations were occurring, you can simply reference the validation list at the top with the built in descriptions (ie, the method names). If through testing you believe one of them isn’t working, you can focus in on that one specific validation and ignore the functionality of the rest of the class.
This ability to focus at one task of a greater activity is really important for maintaining large applications over time.
Hmm, that “validate” class method is news to me also. Thanks for digging it up!
And even more important, more maintainable, and easier to write tests for. (Which I suppose are both sort of encapsulated by “human readable” :-) )
I’m sobbing :) Somebody stole my purchase with credit cards! Please advice where to file a fraud! I also need a pair of new credit cards, my credit is good or so. Where to go? My friend Liz told she’s got a pretty nice card from Discover and applied at
0% balance transfer crdit cards
I’m sobbing :) Somebody stole my purchase with credit cards! Please advice where to file a fraud! I also need a pair of new credit cards, my credit is good or so. Where to go? My friend Liz told she’s got a pretty nice card from Discover and applied at
0% balance transfer crdit cards
Hello, pal! Could you give me advice? I am in the process of looking for a credit card. I had some credit problems in the past and now I am not sure that my application will be approved. Will a denial damage my credit history? What to do? I am going to apply online at
no bt fees discover discovery advantage member credit card