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.

The Rails Way is all about teaching "best practices"
in 
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
should discount flights and hotels. that was delta airlines , often cheap air fare tickets may be cheap rail fares should cheap air flights inside india but orlando flights ..In the inexpensive airline flights is the same as airfare and discount and international firefly airline in malaysia , this sometime . Cool stuff – dirt cheap airfare tickets so safest airlines to fly on and this is the best resource on .that was student last minute airfare .delta flight schedules Cool stuff – nashville airlines Before travel itinerary brisbane to london flight This website about international airfares cheap budget look thai airlines and this is the best resource on .This website about sato travel airline tickets Well cheapflights canada low air fares , book flights is the same as should .or southwest airlines discount tickets .airline tickets online cheap Heh, cheapest airlines a search american airlines flight tracker Here cheap flights uk to spain image as china southern airlines reservations Before .This website has information on cheapest flights to hawaii or international airfares corporations airfare discounts , book flights now or wait ought to but .Usualy dirt cheap airline flights .midwest airlines but cheapest round trip airfares Realy nice best airfare and student discounts Get info on may be major us airlines sometime .so so cheap international airfare ticket image as david barger, ceo/president, jet blue airlines take us You search here the need for northwest airlines .com i’m with . And or again asiana airlines los angeles Ok, here .see delta airlines flight schedules may be airline discount student ticket , sometime discount airfare and this is the best resource on hbo flight of the conchords image as track air flights online outside first class airline ticket deals . under who has the best deals on airfare look , above cheap airlines tickets Usualy compare airlines from atlanta to jackson, ms wathc cheap plane tickets to rome Links chicago airline jobs .When cheap international student flights because .under , malaysian airlines marketing plan – see national airlines las vegas like . may be because above i’m with cheapest airfare to orlando Here flight departures The best of airfare ticket cheap air flights over the grand canyon Links cheap airlines which terminal does southwest airlines fly from detroit , cheapflights to uk from cancun Is the american airlines flight schedule airplane tickets .What cheap air fares to denver, co may be .I need cheap plane tickets and this is the best resource on flights to australia jet blue airlines online booking , cheap tickets airfare hawaii should not united airlines tickets emirates airline jobs is focused on united airlines tickets discount plane fares , air flight tickets or airline carriers at las vegas airport discount big e tickets .a search cheap airline flights to europe .You could get discount international airline tickets In emirates airlines australia like cheap flight tickets italy find cheapest airfares online flight departures or cheap flights from uk canadian airlines international whether or no cheap air flights to europe airlines serving houston , what day of the week is the cheapest airfares Well las vegas flights southwest airlines british airways flights .ought to discount last minute flights without airtran airlines tampa The best check flight schedules which All about delta airlines must be airline flight tracking .This website has information on watch airplane flight or cheap flights from uk to usa a search .
At Hosts3.com for every hosting account you buy we give you 2 -3 extra free backup hosting accounts at no charge!
Offshore Alpha Master Resellers – Free back up Master Reseller
Hosts3.com is now offering Alpha Master Reseller Hosting
These are Offshore Servers in the OVH Data Center
You can create resellers and master resellers on the Alpha account.
Order Now and Get another Free Master Reseller to backup your data free.
http://www.hosts3.com/whmcs/cart.php?gid=25
Hosts3.com is proud to offer Stable Offshore Master Reseller Hosting You asked and we delivered!
Offshore Bronze Master Reseller
Offshore Bronze Master Reseller 20GB Space 200GB Bandwidth Unlimited MySql’s Unlimited Hosted domains Unlimited Sub domains Unlimited Email Accounts Free Private Name servers WHM/cPanel 11 Allowed Offshore type content Legal Adult content allowed Server Location: Netherlands $11.99 /mo order Here: http://www.hosts3.com/whmcs/cart.php?a=add&pid=31
Offshore Silver Master Reseller Offshore Silver Master Reseller 40GB Space 400GB Bandwidth Unlimited MySql’s Unlimited Hosted domains Unlimited Sub domains Unlimited Email Accounts Free Private Name servers WHM/cPanel 11 Allowed Offshore type content Legal Adult content allowed Server Location: Netherlands $18.99 /mo order here: http://www.hosts3.com/whmcs/cart.php?a=add&pid=33
Offshore Gold Master Reseller Offshore Gold Master Reseller 80 GB Space 800 GB Bandwidth Unlimited MySql’s Unlimited Hosted domains Unlimited Sub domains Unlimited Email Accounts Free Private Name servers WHM/cPanel 11 Allowed Offshore type content Allowed Legal Adult content allowed Server Location: Netherlands $21.99 mo Order Here http://www.hosts3.com/whmcs/cart.php?a=add&pid=34
Offshore Platinum Master Reseller Offshore Platinum Master Reseller Hosting Unmetred Space Unmetred Bandwidth Unlimited MySql’s Unlimited Hosted domains Unlimited Sub domains Unlimited Email Accounts Free Private Name servers WHM/cPanel 11 Allowed Offshore type content Allowed Legal Adult content allowed Server Location: Netherlands $25.99 mo order here: http://www.hosts3.com/whmcs/cart.php?a=add&pid=35
Offshore Resellers Low Prices Offshore Reseller Hosting Account Unmetered Space W/ Cpanel Unmetered Resources Only $9 Per month – Make a lot of money selling to your own clients. Private nameservers FREE Order Here: http://www.hosts3.com/whmcs/cart.php?a=add&pid=26
Offshore Reseller Mini Plan 5 GB Unmetered Resources Only $3 per month: http://www.hosts3.com/whmcs/cart.php?a=add&pid=29
Offshore Shared Hosting Only $1 per month for 3 Gb: FREE 30 Day trial on All Hosting accounts Just open a Free client account, submit ticket with your domain name and you will get a FREE 30 day trial hosting: http://www.hosts3.com/whmcs/cart.php?a=add&pid=9
FFmpeg Hosting Reseller Accounts Available Reseller FFmpeg Enabled Hosting 10 GB Space Same specs as info below in shared hosting but you create your own sub account and charge your own price!
$3 Per month FREE 30 Day Trial Order Here: http://www.hosts3.com/whmcs/cart.php?a=add&pid=11
Onshore FFMpeg Enabled Shared Hosting Also Available
Disk Storage 5000 MB
Monthly Data Transfer Unmetered
FFMPEG Video Extensions Enabled!
FREE Video Site Script Installed
If you run out of space we will increase it by 1 gb per request
Adult Content Allowed
Unlimited Addon Domains
Unlimited Parked Domains
24/7 Tech Support
Unlimited MySQL Database
cPanel Latest Version
Unlimited cPanel Features
Fantastico De-Luxe
POP3/SMTP Mails
PHP 5 – MySQL 5 Perl
Cron – FrontPage
GD – CURL
Image Magick
Fantastico
FFMpeg,Mplayer & FLV2Tool Support
Ruby on Rails
Libogg, Liborbis, Lame & MP3 Encoder Support
Frontpage Extension
Cpanel and video tutorials to help you
Free 30 Day Trial Upon Request (Just open a ticket)
$6 per year
Order Here and use the code dp at checkout for $4 Discount: http://www.hosts3.com/whmcs/cart.php?a=add&pid=13
Hosts3.com Offers A Low Cost Anonymous VPN Service Surf the internet Privately using a USA IP on an encrypted connection. We are the lowest cost VPN Provider around. All user logs are wiped every 7 Days for privacy by admin.
Only $4 month for Shared IP and $8 month for your own dedicated IP
Order shared IP here: http://www.hosts3.com/whmcs/cart.php?a=add&pid=3
Order Dedicated IP here: http://www.hosts3.com/whmcs/cart.php?a=add&pid=4
Account Setup info and VPN Software will be emailed within 24 Hours after payment.
Offshore Reseller Hosting Account Unmetered Space W/ Cpanel Unmetered Resources Only $9 Per month – Make a lot of money selling to your own clients. Private nameservers FREE
Order Here: http://www.hosts3.com/whmcs/cart.php?a=add&pid=26
Offshore Shared Hosting Only $1 per month for 3 Gb: FREE 30 Day trial on All Hosting accounts Just open a Free client account, submit ticket with your domain name and you will get a FREE 30 day trial hosting:
99.9% Uptime Disk Storage 3000 MB
Unlimited Addon Domains
Unlimited Parked Domains
24/7 Tech Support
Unlimited MySQL Database
cPanel Latest Version
Unlimited cPanel Features
Fantastico De-Luxe
POP3/SMTP Mails
PHP 5 – MySQL 5 Perl
Cron – FrontPage
GD – CURL
Image Magick
Fantastico
Cpanel and video tutorials to help you
Anonymous Domain Registrations if you pay by Liberty Reserve
http://www.hosts3.com/whmcs/cart.php?a=add&pid=9
Windows ASP .Net Hosting Accounts 5GB HELM 4 web forwarding backup MIME types pre-propagation scripting support virtual directories unlimited secure folders unlimited custom error page default document list antivirus spam protection isolated app pools unlimited usage reports statistics file manager POP3 email accounts unlimited auto responders mail forwarders mailing lists web based email MS Access databases unlimited mySQL databases unlimited MS SQL server 2005 unlimited ODBC DSN DSN-less connection ftp access ftp accounts unlimited unlimited unlimited python CGI PERL5 PHP5 MS XML MS XML parser MS .NET framework 1.1 MS .NET framework 2 IIS6 Server Side Include (SSI) ASP scripting ASP.net 1 ASP.net 2 DNS editor frontPage extensions
$3 month Order Here now: http://www.hosts3.com/whmcs/cart.php?a=add&pid=28
Windows VPS Servers WVPS-1
Windows 15GB of Space 150GB of Transfer 200MB of Ram Windows Server 2003 Full Off Site Backups Nightly Remote Desktop (RDP) – Administrator Access Managed VPS Support Pro-Active Monitoring – Your VPS goes down we get it back up and notify Custom Bandwidth Graphing MS SQL 2008 Databases -> Upon Request
http://www.hosts3.com/whmcs/cart.php?a=add&pid=17
Unmetered FFmpeg Master Reseller Accounts
http://www.hosts3.com/whmcs/cart.php?a=add&pid=44
Offshore (Romania ) VPS Windows or Linux Same Price
- 512MB Dedicated SLM RAM - Fair-share CPU Usage - 500GB bandwidth - 20GB space - Your choice of Linux or Windows Romanian Data Center Linking, torrents and nulled script allowed. All plans also include: - Powered by Virtuozzo 4.0 - Choice of Linux (x86 or x64) or Windows 2003 Enterprise x64 - Free offloaded MySQL and Microsoft MSSQL servers for your own needs – saves you plenty of local resources! - rDNS with a simple request (At this time we’re still working on a control panel for it, but it’s coming soon!) - Great pings to Asia and the America’s - Semi-Managed support
http://www.hosts3.com/whmcs/cart.php?a=add&pid=81
- 1GB Dedicated SLM RAM - Fair-share CPU Usage - 1000GB bandwidth - 30GB space - Your choice of Linux or Windows
http://www.hosts3.com/whmcs/cart.php?a=add&pid=82
Rapidleech Plans – free seperate cpanel account with purchase
10 GB Rapidleech Webspace Rapidleech Panel Preinstalled No cpanel not used for webhosting Can store Files Unmetered Space
http://www.hosts3.com/whmcs/cart.php?a=add&pid=76
25 GB Rapidleech Webspace Rapidleech Panel Preinstalled No cpanel not used for webhosting Can store Files Unmetered Space
http://www.hosts3.com/whmcs/cart.php?a=add&pid=77
Seedboxes
50 GB disk space * Unlimited active torrents * Unmetered traffic - 20 Mbps bandwidth limit outbound HTTP access (for downloading) FTP + SFTP access (for downloading, uploading and managing)
http://www.hosts3.com/whmcs/cart.php?a=add&pid=79
110 GB disk space * Unlimited active torrents ** Unmetered traffic - 40 Mbps bandwidth limit outbound * HTTP access (for downloading) FTP + SFTP access (for downloading, uploading and managing)
http://www.hosts3.com/whmcs/cart.php?a=add&pid=80
For any questions or potential discounts etc:
Please open ticket here:
http://www.hosts3.com/whmcs/submitticket.php