How can I refactor this oversized Ruby method into smaller bits and have unit tests for them?

Question

This is the existing code. Clearly way too much is going on in this index method and furthermore it's untested (and hard to test in its current form).

  def index
#    @s = ""
#    @now=DateTime.now
    @from = params[:from]? ('01/01/'+params[:from]).to_date : '01/01/1991'.to_date
    @to = params[:to]? params[:to]+'/12/31' : '2099/12/31' # + DateTime.now.year + 100.years#  .year)+100.to_s).to_date
    if params[:search_text_1]
      @srch = params[:search_text_1]
      if params[:search_text_2] # An advanced search
        #   
        # groups...
        @groups_comparison = ' and group_id in (-1'
        if params[:groups]
          params[:groups].each do |group_id|
            @groups_comparison+= ','+group_id
          end 
          @groups_comparison+= ')' 
        end 
        #   
        @srch_2 = params[:search_text_2] # May be null
        @version = (params[:version].to_f) #.is_a?(Integer))? params[:version].to_i : 0 # May be 0
        @version_comparison = '(version_number ' + params[:version_comparison] + ' ' + @version.to_s
        if params[:include_blank_version]  # If not checked doesn't pass.'
          @version_comparison+= " or version_number is NULL or version_number = '' )"
        else
          @version_comparison+= " and version_number is not NULL and version_number <> '' )"
        end 
        @date_comparison = ' and ((content_date is NULL) or (content_date > ' + @from.to_s + '))'   #+ ' and content_date <= ' + @to.to_s + '))'

        case params[:join_operator].downcase
          when /and/
            @conditions =
              [@version_comparison + @groups_comparison + @date_comparison +
              ' and ((url_address LIKE ? or alt_text LIKE ?) and (url_address LIKE ? or alt_text LIKE ?))',
              "%"+@srch+"%", "%"+@srch+"%", "%"+@srch_2+"%", "%"+@srch_2+"%"]
          when /or/
            @conditions =
              [@version_comparison + @groups_comparison + @date_comparison +
              ' and ((url_address LIKE ? or alt_text LIKE ?) or (url_address LIKE ? or alt_text LIKE ?))',
              "%"+@srch+"%", "%"+@srch+"%", "%"+@srch_2+"%", "%"+@srch_2+"%"]
        end 
      else # Just a simple search
        @conditions = ['(url_address LIKE ? or alt_text LIKE ? or version_number LIKE ?)', "%"+@srch+"%", "%"+@srch+"%", "%"+@srch+"%"]
      end 
    else
      @conditions = ''
    end 
    @links = Link.all(:joins => :group, :order => 'groups.group_name, links.position', :conditions => @conditions)

    session[:row_shading] = (params[:row_shading] == 'true') ? 'true' : 'false' rescue 'false'
    session[:full_details] = (params[:full_details] == 'true') ? 'true' : 'false' rescue 'false'
    respond_to do |format|
      format.html # index.html.erb
      format.xml  { render :xml => @links }
    end 
  end 

Answer 1

Create a model to hold all the logic and call that with parameters and have tests for the model methods:

Unit Tests

require 'spec_helper'

describe "Prepare Search methods" do

  describe "PrepareSearch#start_date" do

    subject { PrepareSearch.start_date('2000') }
    it { should == DateTime.new(2000,1,1) }

  end 

  describe "PrepareSearch#end_date" do

    subject { PrepareSearch.end_date('2009') }
    it { should == DateTime.new(2009,12,31) }

  end 

  describe "PrepareSearch#groups" do

    subject { PrepareSearch.groups(['1','2','3']) }
    it { should == " and group_id in (-1,1,2,3)" }

  end 

  describe "PrepareSearch#versions" do

    before(:each) do  
      @versions= {:version => 0, :version_comparison => '>=', :include_blank_version => true}
    end 
    subject { PrepareSearch.versions(@versions) } 
    it { should == " and (version_number >= 0.0 or version_number is NULL or version_number = '' )" }

  end 

  describe "PrepareSearch#dates" do

    subject { PrepareSearch.dates(Date.new(2009), Date.new(2012) ) } 
    it { should == " and ((content_date is NULL) or( (content_date >= \"2009-01-01\") and (content_date <= \"2012-01-01\") ) )" }

  end 

  describe "PrepareSearch#basic_search" do

    subject { PrepareSearch.basic_search("test") }
    it { should == ["(url_address LIKE ? or alt_text LIKE ? or version_number LIKE ?)","%test%","%test%","%test%"] }

  end 

  describe "PrepareSearch#text_search" do

    subject { PrepareSearch.text_search("tests","or","rs") }
    it { should == " and ( (url_address LIKE \"%tests%\" or alt_text LIKE \"%tests%\" or version_number LIKE \"%tests%\") or (url_address LIKE \"%rs%\" or alt_text LIKE \"%rs%\" or version_number LIKE \"%rs%\") )" }

  end 

end

New Model "PrepareSearch"

class PrepareSearch

  def self.start_date(start_year)
    (start_year + '/01/01').to_date
  end 

  def self.end_date(end_year)
    (end_year + '/12/31').to_date
  end 

  def self.groups(groups=[])
    @groups_comparison= ' and group_id in (-1'
    groups.each do |group_id|
      @groups_comparison << ','+group_id
    end 
    @groups_comparison+= ')' 
  end 

  def self.versions(version_information={} )
    @version = version_information[:version].to_f
    @version_comparison = ' and (version_number ' + version_information[:version_comparison] + ' ' + @version.to_s
    if version_information[:include_blank_version]
      @version_comparison+= " or version_number is NULL or version_number = '' )"
    else
      @version_comparison+= " and version_number is not NULL and version_number <> '' )"
    end 
  end 

  def self.dates(from, to) 
    @from= from
    @to= to
     ' and ((content_date is NULL) or( (content_date >= "' + @from.to_s + '") and (content_date <= "' + @to.to_s + '") ) )'
  end 

  def self.text_search(words_1,comparison_operator='',words_2='')
    @words_1 = words_1
    @words_2 = words_2
    @comparison_operator= comparison_operator
    if @words_2.length > 0 
      @text_search = ' and ( (url_address LIKE "%'+@words_1+'%" or alt_text LIKE "%'+@words_1+'%" or version_number LIKE "%'+@words_1+'%") '
      @text_search+= @comparison_operator
      @text_search+= ' (url_address LIKE "%'+@words_2+'%" or alt_text LIKE "%'+@words_2+'%" or version_number LIKE "%'+@words_2+'%") )'
    else
      ' and (url_address LIKE "%'+@words_1+'%" or alt_text LIKE "%'+@words_1+'%" or version_number LIKE "%'+@words_1+'%")'
    end 
  end 

  def self.basic_search(words)
    @words = words
    @conditions = ['(url_address LIKE ? or alt_text LIKE ? or version_number LIKE ?)', "%"+@words+"%", "%"+@words+"%", "%"+@words+"%"]
  end 

end

Refactored index action

  def index
    @from = PrepareSearch.start_date(params[:from] ||= '1991')
    @to = PrepareSearch.end_date(params[:to] ||= '2299')

    if params[:search_text_1st_phrase] # both simple and advanced search use this field.
      @words_1 = params[:search_text_1st_phrase]
      if params[:commit].downcase == 'advanced search'
        @join_operator = params[:join_operator].downcase
        @words_2 = params[:search_text_2nd_phrase]
        @text_search = PrepareSearch.text_search(@words_1, @join_operator, @words_2)
        @groups_comparison = PrepareSearch.groups(params[:groups])
        @version_information = { :version => params[:version], :version_comparison => params[:version_comparison], :include_blank_version => params[:include_blank_version] }
        @version_comparison = PrepareSearch.versions(@version_information) 
        @date_comparison = PrepareSearch.dates(@from, @to)
        @conditions = '1=1' + @groups_comparison+ @version_comparison+ @date_comparison + @text_search
      else
        @conditions = PrepareSearch.basic_search(@words_1)
      end 
    else
      @conditions = ''
    end 
    @links = Link.all(:joins => :group, :include => :group, :order => 'groups.group_name, links.position', :conditions => @conditions)
    session[:full_details] = (params[:full_details] == 'true') ? 'true' : 'false' rescue 'false'
    respond_to do |format|
      format.html
    end 
  end 

Also (next steps):

1. Write controller/integration tests to check actual search results for various combinations of parameters. Needs a bunch of test data. Probably at least 20 links and 4 groups for useful testing data.

2. consider making an object for all the params and passing them to one method call.

Answer 2

There is an easy way to use join and a function to take away the complexity of an in clause concatenation so that the following:

@groups_comparison = ' and group_id in (-1'
if params[:groups]
  params[:groups].each do |group_id|
    @groups_comparison+= ','+group_id
  end 
  @groups_comparison+= ')' 
end 

can be rewritten as:

def comparison_section group_ids
  return if group_ids.nil?

  groups_comparison + "and group_id in (-1, #{group_ids.join(',')} )"
end

Also, in general, a good way to test things is to put them into functions with parameters so:

def format_from
  params[:from]? "01/01/#{params[:from]}".to_date : '01/01/1991'.to_date
end

would be harder to test than

def format_from(from)
  from ? "01/01/#{from}".to_date : '01/01/1991'.to_date
end

because you would need to set params to test the first function. Also, since the first one doesn't rely on explicitly passing arguments, the value could potentially be changed before the function *format_from* is used causing a difficult to catch bug.

One of the reasons that you are seeing such complexity here is that SQL queries in rails just simply aren't supposed to be generated this way. Putting a query like this into a separate library or potentially as a series of scopes on your model may help in reducing some of the complexity. Also, running tests on models/libraries is less expensive time-wise than running tests on controllers.