[Ruby] code review

[Ryan Davis]

On Oct 29, 2006, at 5:32 PM, Ofer Matan wrote:

> The following code is a class method for an
> ActiveRecord that gets all objects with a particular
> filter, but I wanted it also to allow all the other
> fancy find options that ActiveRecord provides:
>
>   MASTER_ID_IS_NULL = " and master_id is NULL"
>   def self.masters(options={})
>     default_options = {:conditions => 'true'}
>     options = default_options.merge(options)
>     if options[:conditions].is_a?(Array) then
>       options[:conditions][0] += MASTER_ID_IS_NULL
>     else #string
>       options[:conditions] += MASTER_ID_IS_NULL
>     end
>     find(:all,options)
>   end

this seems like an awful lot of work to wrap up one find and make  
sure that master_id is null. I'd look at the find_all_with_XXX  
methods and see how much you're actually gaining by having this code.  
That said, a couple suggestions:

+ get rid of the default_options variable. It doesn't really help  
readability.

+ try:

options[:conditions] = Array(options[:conditions])
options[:conditions][0] += MASTER_ID_IS_NULL