This post originated from an RSS feed registered with Ruby Buzz by Ryan Davis. Original Post: Argh! Stop being "clever"! Feed Title: Polishing Ruby Feed URL: http://blog.zenspider.com/index.rdf Feed Description: Musings on Ruby and the Ruby Community... Latest Ruby Buzz Posts Latest Ruby Buzz Posts by Ryan Davis Latest Posts From Polishing Ruby

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, columns| b and columns.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? {|columns| columns.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? {|columns| columns.include? fk}

Measure the change:

1. 1 line, 132 chars, 131 avg len
2. 2 line, 140 chars, 69 avg len
3. 3 lines, 180 chars, 59 avg len
4. 3 lines, 163 chars, 53.3 avg len
5. 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)

Sure does pay to not be clever...