Blog, Programming, Rails, Ruby

Overriding find for Cleaner Code

Posted September 19, 2007

When I first added search capabilities to this blog, I did it by overriding the current_objects method in the controller.

For those who haven’t yet discovered the joys of make_resourceful, current_objects is a special method used by to retrieve a collection of models that the index action will display. By default it looks (sort of) like this:

def current_objects
  @current_objects ||= Post.find(:all)
end

If you override it, everything that acts on the collection object uses whatever your function returns. The rule of thumb is: if you want to change the collection, redefine current_objects. So I did.

def current_objects
  @current_objects ||=
    if params[:query]
      term = "<span>#{params[:query]}</span>" 
      Post.find(:all, :order => 'created_at DESC',
                :conditions => ['content LIKE ? OR title LIKE ?', term, term])
    else
      Post.find(:all, :order => 'created_at DESC', :limit => 6)
    end
end

A little more verbose, but not really enough to set off the refactoring alarm. Its ugliness only became fully apparent just recently, when I wanted to add support for tags. After working out some SQL to eager-load as much data as possible, current_objects ended up looking like this:

def current_objects
  @current_objects ||=
    begin
      opts = {:order => 'posts.created_at DESC', :limit => 6, :include => [:comments, :tags]}

      if tags
       opts[:joins] = <<-END
         INNER JOIN posts_tags AS inner_posts_tags ON posts.id = inner_posts_tags.post_id
         INNER JOIN tags AS inner_tags ON inner_tags.id = inner_posts_tags.tag_id
       END
       opts[:conditions] = [tags.map { 'inner_tags.name = ?' }.join(' OR '), *tags]
      end

      if params[:query]
        term = "%#{params[:query]}%" 
        cond = "posts.content LIKE ? OR posts.title LIKE ?" 

        opts.delete :limit
        if opts[:conditions]
          opts[:conditions][0] << " AND (#{cond})" 
          opts[:conditions] << term << term
        else
          opts[:conditions] = [cond, term, term]
        end
      end

      Post.find(:all, opts)
    end
end

def tags
  return if params[:tag].nil? and params[:tags].nil?
  @tags ||= "#{params[:tag]},#{params[:tags]}".split(',').map(&:downcase).map(&:strip).reject(&:empty?)
end

Yikes. That’s one ugly method, and a huge amount of logic to have in the controller. Remembering the mantra¹, I decided to move as much of it as possible into the Post model.

Upon deciding to do so, I ran into a new issue: how to put the code into Post. I could have just copied it verbatim to some sort of Post.find_by_tag_and_query method. I could split up Post.find_by_tag and Post.find_by_query, and try to find some way to mix searching and tagging that didn’t go into infinite loops.

Neither of these possibilities appealed to me. They seemed like they’d just shift the ugly code around a little. It would remain just as messy as it was before. I needed to put on my API-creation hat and get thinking.

One handy trick for designing APIs that I’ve used before is to ask myself, “What API do I wish I could use?” Sometimes I even write code snippets calling the imaginary API. Then I code it up so that my code snippets become functional².

I didn’t actually write out the new controller method, but I did have it in my head. For your amusement, here’s the current_objects method I ended up writing³, using my new API:

def current_objects
  @current_objects ||=
    Post.find(:all, :order => 'posts.created_at DESC', :limit => params[:query] ? nil : 6,
              :include => [:comments, :tags], :tags => tags, :query => params[:query])
end

See, the API I wished I had here was actually one that already existed: Post.find. But there was a problem with the pre-existing one: it didn’t accept all the options I wanted to give it. I needed to add :tags and :query options.

I did this, of course, by overriding the find method. I think this is an excellent way to add support for query abstractions like tags and searching. It keeps the logic in the model, it works everywhere in your app, and, maybe most importantly, the additions look completely natural next to the other options.

My first go at overriding find was pretty standard alias_method_chain stuff. I also created a helper function to nondestructively add a condition to the options hash. It all looked something like this:

class Post < ActiveRecord::Base
  class << self
    def find_with_tags(*args)
      options = extract_options_from_args!(args)
      if tag = options.delete(:tags)
        options[:select] ||= 'posts.*'
        options[:joins]  ||= ''
        options[:joins]  << <<-END
          INNER JOIN posts_tags AS inner_posts_tags ON posts.id = inner_posts_tags.post_id
          INNER JOIN tags AS inner_tags ON inner_tags.id = inner_posts_tags.tag_id
        END
        add_to_conditions(options, tags.map { 'inner_tags.name = ?' }.join(' OR '), *tags)
      end
      find_without_tags(*(args + [options]))
    end
    alias_method_chain :find, :tags

    def find_with_query(*args)
      options = extract_options_from_args!(args)
      if query = options.delete(:query)
        if query.empty?
          add_to_conditions(options, 'false')
        else
          term = "%#{query}%" 
          add_to_conditions(options, "posts.content LIKE ? OR posts.title LIKE ?", term, term)
        end
      end
      find_without_query(*(args + [options]))
    end
    alias_method_chain :find, :query

    protected

    def add_to_conditions(options, condition, *args)
      condition = args.empty? ? condition : [condition, *args]
      if options[:conditions].nil?
        options[:conditions] = condition
      else
        options[:conditions] = sanitize_sql(options[:conditions]) + " AND (#{sanitize_sql(condition)})" 
      end
    end
  end
end

That’s a lot of repetition for an app trying to stay DRY, though. I decided to use some metaprogramming to clean it up:

class Post < ActiveRecord::Base
  class << self
    def handle_find_option(name, &block)
      eigenclass = class << self; self; end
      eigenclass.send :define_method, "find_with_#{name}_handled" do |*args|
        options = extract_options_from_args!(args)
        if option = options.delete(name)
          block[options, option]
        end
        send("find_without_#{name}_handled", *(args + [options]))
      end
      eigenclass.send :alias_method_chain, :find, "#{name}_handled" 
    end
  end
end

handle_find_option does all of the boilerplate work for adding an option handler to find. It creates a function that grabs the option value and, if it exists, calls the handler. Then it passes the resulting modified options hash to the old find. It even aliases the methods.

Here’s what my handlers look like now:

class Post < ActiveRecord::Base
  handle_find_option(:tags) do |options, tags|
    options[:select] ||= 'posts.*'
    options[:joins]  ||= ''
    options[:joins]  << <<-END
      INNER JOIN posts_tags AS inner_posts_tags ON posts.id = inner_posts_tags.post_id
      INNER JOIN tags AS inner_tags ON inner_tags.id = inner_posts_tags.tag_id
    END
    add_to_conditions(options, tags.map { 'inner_tags.name = ?' }.join(' OR '), *tags)
  end

  handle_find_option(:query) do |options, query|
    if query.empty?
      add_to_conditions(options, 'false')
    else
      term = "%#{query}%" 
      add_to_conditions(options, "posts.content LIKE ? OR posts.title LIKE ?", term, term)
    end
  end
end

And now my current_object is sleek and functional, and I can find posts by tag or query anywhere in my app, using the normal find syntax.

¹ Fat models, skinny controllers. As much logic should be placed into the model as possible, leaving the controllers to do only the minimal work of hooking up models and views.

² This is actually essentially what BDD advocates, except writing specs using the imaginary code.

³ There’s actually a subtle bug in that function that I fixed later on. See if you can spot it.