I started reading Giles Bowkett’s blog a little while ago. He has a lot of rants, and it’s fun. I would have commented on his blog but it seems to be restricted. In a recent article he criticized someone for their custom dynamic finder, attacking them on a number of fronts: SQL performance, architecture design, and readability. But he forgot to check his code in irb, claiming that:

#merge() is a Hash method. It adds new keys to an existing hash based on another existing hash. Unfortunately, it does so destructively. If you have two values defined for the :conditions key in two distinct hashes and you merge them, only one :conditions value will survive the harrowing ordeal. #merge() is Darwinian - it’s survival of the fittest in there.

Amusing, but wrong:

  1.  
  2. >> h={:conditions => "true"}
  3. => {:conditions=>"true"}
  4. >> g=h.merge(:conditions => "false", ‘order’ => ‘email desc’)
  5. => {:conditions=>"false", ‘order’=>"email desc"}
  6. >> h
  7. => {:conditions=>"true"}
  8. >> g
  9. => {:conditions=>"false", ‘order’=>"email desc"}
  10.  

You were thinking of update and merge!. Also, he quotes Assaf as saying:

`*opts` is a bug, lacking an understanding of how Ruby handles arguments. `*opts` refers to all other arguments you pass, the vararg equivalent. But when you call `find_by_email(”…”, :limit=>5, ‘order’=>”name”)` the last two “arguments” (limit and order) are actually keys in a hash that’s passed as a single argument. So anything you pass to find_by_email, the original, that falls inside *opts, would be passed to find after the last argument it expects (the hash of options), which should, if implemented correctly, cause find to fail with an error.

That’s correct up to the last sentence. Let’s look at the code in question:

  1.  
  2.   class User < ActiveRecord::Base
  3.     def self.find_by_email(email, *opts)
  4.       with_scope(:find => { :conditions => [‘lower(email) = lower(?)‘, email] }) do
  5.         find(:first, *opts)
  6.       end
  7.     end
  8.   end
  9.  

This will work. There’s nothing wrong with using the splat operator here other than the fact that it’s going to make this method slower than it out to be. Here’s a better implementation:

  1.  
  2.   class User < ActiveRecord::Base
  3.     def self.find_by_email(email, options={})
  4.       with_scope(:find => { :conditions => [‘lower(email) = lower(?)‘, email] }) do
  5.         find(:first, options)
  6.       end
  7.     end
  8.   end
  9.  

C’mon Giles, you’re better than this.