views:

31

answers:

2

Need some help refactoring this if/else block that builds the conditions for a find query.

if params[:status] && params[:carrier]
  conditions = ["actual_delivery IS NOT NULL AND actual_delivery > scheduled_delivery AND status_id = ? AND carrier_id = ?", status.id, carrier.id]
elsif params[:status]
  conditions = ["actual_delivery IS NOT NULL AND actual_delivery > scheduled_delivery AND status_id = ?", status.id]
elsif params[:carrier]
  conditions = ["actual_delivery IS NOT NULL AND actual_delivery > scheduled_delivery AND carrier_id = ?", carrier.id]
else
  conditions = ["actual_delivery IS NOT NULL AND actual_delivery > scheduled_delivery"]
end

@packages = Package.find(:all, :conditions => conditions)
A: 

You could do:

conditions = "actual_delivery IS NOT NULL AND actual_delivery > scheduled_delivery"
conditions += " AND status_id = #{status.id}" if params[:status]
conditions += " AND carrier_id = #{carrier.id}" if params[:carrier]

@packages = Package.all(:conditions => [conditions])
jordinl
@jordinl performance wise << better than +=.
krunal shah
+1  A: 

I recommend creating a scope in your model, to take care of the first part of your query that is always the same in this action:

class Package < ActiveRecord::Base
  named_scope :late_deliveries, :conditions => "actual_delivery IS NOT NULL AND actual_delivery > scheduled_delivery"
end

Now you can refactor your action like this:

def index
  conditions = {}
  [:status, :carrer].each{|param| conditions[param] = params[param] if params[param]}

  @packages = Package.late_deliveries.find(:conditions => conditions)
end

If :carrier and :status are the only two parameters to this action, then it's even simpler:

def index
  @packages = Package.late_deliveries.find(:conditions => params)
end

I hope this helps!

Jaime Bellmyer
Can scopes work with something like `will_paginate`?
Shpigford
Yes, it does. Just replace ".find" with ".paginate" in the examples above, adding in the page/per_page parameters as needed.
Jaime Bellmyer