Refactoring: Implementing filtering June 16th, 2008

For a project at Openminds we needed an implementation for filtering some data from a list with the aid of checkboxes. Our first solution looked like this:

def index
  session[:order_filter] = params[:filter] if params[:filter] 
  session[:order_filter] ||= { :active => '1', :inactive => '1' } 

  if session[:order_filter][:active] == '0' && session[:order_filter][:inactive] == '0'
    @orders = []
  elsif session[:order_filter][:active] == '0'
    @orders = @user.orders.find(:all, :conditions => {:state => 'active'})
  elsif session[:order_filter][:inactive] == '0'
    @orders = @user.orders.find(:all, :conditions => {:state => 'inactive'})
  else
    @orders = @user.orders.find(:all)
  end
end

So this code takes the filter-parameters from params, or initializes them from defaults, and then does the correct finds.

In the next iteration, I stumbled onto this code and thought to myself “This can be done better”. After making sure the index action was sufficiently covered with tests, I had a go at it, this is what I came up with:

def list
  if filter.active? && filter.inactive?
      @orders = @user.orders.all
  elsif filter.active?
     @orders = @user.orders.active
  elsif filter.inactive?
    @orders = @user.orders.inactive
  else
    @orders = []
  end
end

protected
def filter
  return @filter if @filter
  session[:order_filter] = params[:filter] if params[:filter]
  session[:order_filter] ||= { :active => '1', :inactive => '1' }

  @filter = SearchFilter.new("Order", session[:order_filter])
end
helper_method :filter

# search_filter.rb
class SearchFilter
  attr_reader :name

  def initialize name, hash
    @name = name
    hash.each do |key, value|
      instance_eval <<METHOD
        def #{key}?
          #{value} == 1
        end
METHOD
    end
  end
end

This code is more readable in my opinion. I extracted the session-storing and parameter-juggling to the filter-method, which then returns a SearchFilter object. Thanks to SearchFilter I can now query filter for set values in a nice way. I can also call filter in my views, which is nicer then calling session[:order_filter][:active]. I also used named_scope instead of doing the finds manually, which improves readability a lot too!

How would you refactor this?

tags: , , l

2 Responses to “Refactoring: Implementing filtering”

  • about 1 month ago Edwin V. said

    Absolutely a nice refactoring example Jan. It’s nice to see how much more readability can be achieved just by adding an extra class to the project.

    Personally I would implement the actice? and inactive? methods with method_missing. Just trying to avoid ‘eval’ methods as much as possible, you never know what the input from your users might be.

    (And is your example code working? I expect you to get some unexpected behaviour due to a string == integer comparison.)

  • about 1 month ago Jan De Poorter said

    I was thinking about implementing it with method_missing, but needed a challenge ;-)

    The method is working because the “1” gets parsed as 1 in the code, so we get

    def something?
  1 == 1
end

Leave a Reply