This is inspired by Ruby, but could apply to Perl as well.
Situation: class with dynamic, on the fly class method definitions of the form
[:role1, :role2, :role3].each do |m|
class_eval < <-EOV
def self.#{m}_email_addresses
Role.find_by_title('#{m}').users.inject([]) do |memo, u|
memo.push( u.email ) unless u.email.empty?
end.delete_if{ |i| i.nil? or i.empty?}
end
EOV
end
What this does is defines some class methods role1_email_addresses, role2_email_addresses, etc on the fly.
Works perfectly, until the first element returned in users is actually an empty email.
This is an example of writing clever code for succinctness (which is rampant in ruby circles). The problem is this code has bugs, and in order to fix it you have to rewrite it or add extra checks and workarounds. Which means the code is no longer succinct nor clever.
In my recent experience, that’s all that overly-clever code gets you. You just end up making extra work for yourself (or the maintainer) down the line.
4 responses so far ↓
1 Jeremy Friesen // May 2, 2007 at 3:28 pm
I believe I see your problem, its with the inject statement; the last line of inject must return the mem variable.
No Worky
[1,2,nil,3].inject([]) do |m,v|
m
2 brink // May 2, 2007 at 3:44 pm
Thanks, yeah I got that particular issue solved, however it also has a bug at the end on the delete_if, which fails if there are no actual results of the preceding block.
My point is that the code is too clever. It’s not immediately obvious that there are bugs in it, because it’s obfuscated by the concatenation of methods.
Don’t get me wrong, it sort of appeals to me to have an essential one-liner, but more and more I’m of the opinion that verbosity is better in the long run.
3 Jeremy Friesen // May 2, 2007 at 3:56 pm
Agreed, too much work in a single method is a dangerous thing.
4 Sidu // Feb 27, 2008 at 3:45 am
Not to mention the fact that it’s hard to test that big blob of generated code. Meta-programmatically generating untestable code is the path to the dark side.
Leave a Comment