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...
