My theme for this year is shaping up to be "don't be clever, it doesn't suit you". Here is some real code we found and how it can quickly and easily be cleaned up to be much more readable, more correct, and more efficient all at the same time. (indeed, it took more time to benchmark and write this post than it did to fix the code)
The original code in all its clever glory:
if MODELS.keys.inject(true) {|b, klass| b and klass.constantize.columns.map(&:name).include? association.options[:foreign_key]}
pull out the foreign key, since there is no reason to do that in a loop:
fk = association.options[:foreign_key]
if MODELS.keys.inject(true) {|b, klass| b and klass.constantize.columns.map(&:name).include? fk}
pull out columns to improve readability:
fk = association.options[:foreign_key]
columns = MODELS.keys.map { |key| key.constantize.columns.map(&:name) }
if columns.inject(true) {|b, column| b and column.include? fk}
Oh look, now that we can read it, inject is totally wrong and slow:
fk = association.options[:foreign_key]
columns = MODELS.keys.map { |key| key.constantize.columns.map(&:name) }
if columns.all? {|column| column.include? fk}
Finally, while that map symbol to_proc hack sure looks cute, it sucks:
fk = association.options[:foreign_key]
columns = MODELS.keys.map { |key| key.constantize.columns.map { |c| c.name } }
if columns.all? {|column| column.include? fk}
Measure the change (slightly wrong after minor edits):
- 1 line, 132 chars, 131 avg len
- 2 line, 140 chars, 69 avg len
- 3 lines, 180 chars, 59 avg len
- 3 lines, 163 chars, 53.3 avg len
- 3 lines, 163 chars, 54.3 avg len
And finally, (rough) benchmarks:
\# of iterations = 100000
iter user system total real
null_time 0.020000 0.000000 0.020000 ( 0.012905)
benchmark-1 11.350000 0.030000 11.380000 ( 11.564039)
benchmark-2 10.900000 0.020000 10.920000 ( 10.975134)
benchmark-3 10.720000 0.010000 10.730000 ( 10.928107)
benchmark-4 10.520000 0.030000 10.550000 ( 11.300934)
benchmark-5 3.260000 0.010000 3.270000 ( 3.275238)
(nevermind: don't look at the times, that wasn't the point... those that focused on the times missed the point.)
Sure does pay to not be clever...
It not only pays, but it's usually clever not to be clever.
Besides the performance, consider the time saved when you have to read that code sometime in the future.
Would it be correct to say that the biggest gain in performance was in unrolling the to_proc hack?
Well, there's a patch brewing to make Symbol#to_proc a lot faster (very close to the explicit block), and I've tossed the equivalent into most of my projects. It's also more or less the same as Ruby 1.9's version. The main difference is that it doesn't take arguments, which is almost never of any use. (The only example I've ever seen is [1,2,3].inject(&:+), and I find that unspeakably revolting.)
You might want to check files I've uploaded at http://dev.rubyonrails.org/ticket/8818
Kamal/Djur/Pritik: you guys don't get it. read rick's comment. he gets it.
I've never liked inject, precisely because Ruby programmers have this odd tendency to go out of their way to use it inappropriately.
That aside, I always question any line of code that's greater than 80 characters. I wonder how much of this single-line cleverness is caused by the Rails source code style. You may laugh, but I believe there's a connection.
I think that one of the main reasons for code like this is that cleverness can sometimes be confused with elegance, as strange as that sounds. Someone new to ruby sees all of these tricks that ruby can do, then tries to squeeze as many of them into his code as he can.
Writing better Ruby is part philosophy and part knowing about lots of quirks. For most errors you can say "THIS IS TOTALLY WRONG" and I say "okayy..." and try to remember it, though I could never deduce that it was wrong on my own. Had it not been pointed out, I probably would have gone through life not knowing that the map symbol to_proc trick is slow or that '=~' is better than /../.match.
How about a 'strict-ruby' that throws warnings when people do things they shouldn't? ;-)
Oh, I get it. I've had to untangle my share of "cleverness", especially in Rails apps, and often in my own code a month later. My rule of thumb is no more than one block per line, two statements per block, and three levels per successive method call (thus, a.b.c is OK in most cases but a.b.c.d.map(&:hurr).sum(&:duh) is not).
The reason I was addressing Symbol#to_proc was because, well, removing it was the greatest performance increase you saw while being (to my mind) the least useful change in terms of readability.
On the other hand, if you think that &:sym is inherently bad because it's unreadable or confusing or whatever, I see where you're coming from, but I disagree. Although I'd love to see a Prototype-style Enumerable#pluck; that'd eliminate the most common use case for &:sym, which is with Enumerable#map.
Im not sure how breaking it into three lines would make this:
if MODELS.keys.inject(true)
Any easier to spot the problems. As soon as I see an inject(true) call I immediatly know the person who wrote that code didn't understand there are better methods, like any?, all?, detect?, and that it needs to be refactored, no matter how complicated the rest of the line is.
Also, it looks like your moving towards more of a procedural way of doing things. There are plenty of opportunities in the above code to do a little extract method and make this stuff even more clear. When I start breaking things apart I always try to remember: This function should only be doing ONE thing. I know its a little nitpicky but its important.
Performance is irrelevant; it's a question of maintainability.
Evan Weaver,
You are a complete and total idiot. It can't get simpler than that.
Maintainability?!?!?! You're arguing maintainability?!?!?!?!!!!
So do you consider ruby core to be a bunch of idiots as well for http://eigenclass.org/hiki/Changes+in+Ruby+1.9#l129 ? Or it's just time to grow up may be.