Purchase controller search method

Question

I've been trying to simplify a search method in a project that I've taken over. Is it possible to make the code any cleaner than I already have done?

purchase_controller#search (original)

def search
 parms = search_params
 @purchases = []
 if parms[:active].nil?
 active = false
 else
 active = parms[:active] == 'true'
 end
 uid = parms[:customer_id]
 pvid = parms[:provider_id]
 pat = parms[:patient_id]
 num_records = 10 if parms[:limit].nil?
 num_records = parms[:limit].to_i unless parms[:limit].nil?
 offset = 0 if parms[:offset].nil?
 offset = parms[:offset].to_i unless parms[:offset].nil?
 if parms[:active].nil?
 # all three
 @purchases = Purchase.where(customer_id: uid, provider_id: pvid, patient_id: pid).all.order(created_at: :desc).limit(num_records).offset(offset) if !uid.nil? && !pvid.nil? && !pat.nil?
 # two of three
 @purchases = Purchase.where(provider_id: pvid, patient_id: pid).all.order(created_at: :desc).limit(num_records).offset(offset) if !pvid.nil? && !pat.nil? && uid.nil?
 @purchases = Purchase.where(customer_id: uid, patient_id: pid).all.order(created_at: :desc).limit(num_records).offset(offset) if !uid.nil? && !pat.nil? && pvid.nil?
 @purchases = Purchase.where(customer_id: uid, provider_id: pvid).all.order(created_at: :desc).limit(num_records).offset(offset) if !uid.nil? && !pvid.nil? && pat.nil?
 # one of three
 @purchases = Purchase.where(customer_id: uid).all.order(created_at: :desc).limit(num_records).offset(offset) if !uid.nil? && pvid.nil? && pat.nil?
 @purchases = Purchase.where(provider_id: pvid).all.order(created_at: :desc).limit(num_records).offset(offset) if uid.nil? && !pvid.nil? && pat.nil?
 @purchases = Purchase.where(patient_id: pid).all.order(created_at: :desc).limit(num_records).offset(offset) if uid.nil? && pvid.nil? && !pat.nil?
 # none of three
 @purchases = Purchase.all.order(created_at: :desc).limit(num_records).offset(offset) if uid.nil? && pvid.nil? && pat.nil?
 else
 # all three
 @purchases = Purchase.where(customer_id: uid, provider_id: pvid, active: active, patient_id: pid).offset(offset).all.order(created_at: :desc).limit(num_records) if !uid.nil? && !pvid.nil? && !pat.nil?
 # two of three
 @purchases = Purchase.where(provider_id: pvid, active: active, patient_id: pid).all.order(created_at: :desc).limit(num_records).offset(offset) if !pvid.nil? && !pat.nil? && uid.nil?
 @purchases = Purchase.where(customer_id: uid, active: active, patient_id: pid).all.order(created_at: :desc).limit(num_records).offset(offset) if !uid.nil? && !pat.nil? && pvid.nil?
 @purchases = Purchase.where(customer_id: uid, provider_id: pvid, active: active).all.order(created_at: :desc).limit(num_records).offset(offset) if !uid.nil? && !pvid.nil? && pat.nil?
 # one of three
 @purchases = Purchase.where(customer_id: uid, active: active).all.order(created_at: :desc).limit(num_records).offset(offset) if !uid.nil? && pvid.nil? && pat.nil?
 @purchases = Purchase.where(provider_id: pvid, active: active).all.order(created_at: :desc).limit(num_records).offset(offset) if uid.nil? && !pvid.nil? && pat.nil?
 @purchases = Purchase.where(active: active, patient_id: pid).all.order(created_at: :desc).limit(num_records).offset(offset) if uid.nil? && pvid.nil? && !pat.nil?
 # none of three
 @purchases = Purchase.where(active: active).all.order(created_at: :desc).limit(num_records).offset(offset) if uid.nil? && pvid.nil? && pat.nil?
 end
 @purchases.each do |hc|
 hc.stripe_charge_id = nil
 hc.stripe_transaction_id = nil
 end
 render :index, status: :ok, json: @purchases
 end

purchases_controller#search (improved ?)

def search
 parms = search_params
 @purchases = []
 if parms[:active].nil?
 active = false
 else
 active = parms[:active] == 'true'
 end
 uid = parms[:customer_id]
 pvid = parms[:provider_id]
 pat = parms[:patient_id]
 num_records = 10 if parms[:limit].nil?
 num_records = parms[:limit].to_i unless parms[:limit].nil?
 offset = 0 if parms[:offset].nil?
 offset = parms[:offset].to_i unless parms[:offset].nil?
 base_scope = Purchase.order(created_at: :desc).offset(offset).limit(num_records)
 if parms[:active].nil?
 # all three
 @purchases = base_scope.where(customer_id: uid, provider_id: pvid, patient_id: pid) if !uid.nil? && !pvid.nil? && !pat.nil?
 # two of three
 @purchases = base_scope.where(provider_id: pvid, patient_id: pid) if !pvid.nil? && !pat.nil? && uid.nil?
 @purchases = base_scope.where(customer_id: uid, patient_id: pid) if !uid.nil? && !pat.nil? && pvid.nil?
 @purchases = base_scope.where(customer_id: uid, provider_id: pvid) if !uid.nil? && !pvid.nil? && pat.nil?
 # one of three
 @purchases = base_scope.where(customer_id: uid) if !uid.nil? && pvid.nil? && pat.nil?
 @purchases = base_scope.where(provider_id: pvid) if uid.nil? && !pvid.nil? && pat.nil?
 @purchases = base_scope.where(patient_id: pid) if uid.nil? && pvid.nil? && !pat.nil?
 # none of three
 @purchases = base_scope if uid.nil? && pvid.nil? && pat.nil?
 else
 active_scope = base_scope.where(active: active)
 # all three
 @purchases = active_scope.where(customer_id: uid, provider_id: pvid, patient_id: pid) if !uid.nil? && !pvid.nil? && !pat.nil?
 # two of three
 @purchases = active_scope.where(provider_id: pvid, patient_id: pid) if !pvid.nil? && !pat.nil? && uid.nil?
 @purchases = active_scope.where(customer_id: uid, patient_id: pid) if !uid.nil? && !pat.nil? && pvid.nil?
 @purchases = active_scope.where(customer_id: uid, provider_id: pvid) if !uid.nil? && !pvid.nil? && pat.nil?
 # one of three
 @purchases = active_scope.where(customer_id: uid) if !uid.nil? && pvid.nil? && pat.nil?
 @purchases = active_scope.where(provider_id: pvid) if uid.nil? && !pvid.nil? && pat.nil?
 @purchases = active_scope.where(patient_id: pid) if uid.nil? && pvid.nil? && !pat.nil?
 # none of three
 @purchases = active_scope if uid.nil? && pvid.nil? && pat.nil?
 end
 @purchases.each do |hc|
 hc.stripe_charge_id = nil
 hc.stripe_transaction_id = nil
 end
 render :index, status: :ok, json: @purchases
 end

Answer 1

Whenever my controllers become too large and the logic too complex, I push that logic down into one of a couple layers:

1. The model
2. A Command or Service object

Pushing Behavior Into The Model

The first thing I see that could be pushed into the model is this code:

@purchases.each do |hc|
 hc.stripe_charge_id = nil
 hc.stripe_transaction_id = nil
end

The two lines that set the stripe_charge_id and stripe_transaction_id to nil should be a method on the Purchase model:

class Purchase < ActiveRecord::Base
 def clear_stripe_info
 stripe_charge_id = nil
 stripe_transaction_id = nil
 end
end

The rest of it should be pushed into some sort of service or command object. I have a personal disdain for "service" objects because of the same problems with naming things "SomethingManager". I prefer the command pattern for these sorts of things.

Moving Search Behavior Into A Command Object

All of the search logic can be encapsulated in the command object. The idea behind this is to:

1. Simplify your controller
2. Make the search logic reusable outside the context of an MVC web application
3. Make it easier to unit test your controller

On top of that, we can take advantage of the ActiveRecord query interface to further reduce the lines of code.

Simplifying the Controller

First, let's look at the simplified controller:

def search
 command = SearchPurchasesCommand.new search_params
 @purchases = command.execute
 render :index, status: :ok, json: @purchases
end

Ideally, we want just three lines of code.

1. Create the SearchPurchasesCommand object and pass it parameters
2. Tell the command object: "Just do whatever it is you do to give me search results"
3. Render the view and be done with it

Easy-peasey, rice and cheesey. It makes testing this controller much easier because, you only need to mock the search_params and the return value of the command object, instead of needing the full weight of ActiveRecord and a relational database just for a unit test.

Creating the SearchPurchasesCommand Class

Next, the implementation of the SearchPurchasesCommand class that encapsulates all of the craziness of searching for purchases:

class SearchPurchasesCommand
 def initialize(params = {})
 @active = params[:active] == 'true' unless params[:active].nil?
 @num_records = params[:limit].nil? ? 10 : params[:limit].to_i
 @offset = params[:offset].nil? ? 0 : params[:offset].to_i
 @customer_id = params[:customer_id]
 @provider_id = params[:provider_id]
 @patient_id = params[:patient_id]
 end
 def execute
 query = Purchases.order(created_at: :desc)
 .limit(@num_records)
 .offset(@offset)
 query = query.where(customer_id: @customer_id) unless @customer_id.nil?
 query = query.where(provider_id: @provider_id) unless @provider_id.nil?
 query = query.where(patient_id: @patient_id) unless @patient_id.nil?
 query = query.where(active: @active) unless @active.nil?
 query.all.each { |purchase| purchase.clear_stripe_info }
 end
end

The initialize method extracts the params into usable variables. A simple Ternary operator is all that's needed to convert the offset and limit to integers rather than two lines of code for each case.

The execute method is where all the work is done. The first thing we do is get a query object from ActiveRecord. The order, limit and offset are common to all branches of your code, so we start there. Now that we have a query object, it's just a simple matter of testing the other conditions for nil before adding that condition to the query.

Lastly, before we return the array of results, we loop over each one and call the Purchase#clear_stripe_info method we defined earlier.

Answer 2

I think the purchases conditions can be simplified vastly if you just prepare the hash before. Something like this (off the top of my head):

where_hash = {}
{
 :uid => :customer_id,
 :pvid => :provider_id,
 :pat => :patient_id
}.each do |the_var, hash_key|
 the_value = binding.local_variable_get(the_var) # Ruby >= 2.1
 # binding.eval("the_value = '#{the_var}'") # Ruby < 2.1
 where_hash[hash_key] = the_value unless the_value.nil?
end
my_limit = my_offset = my_order = nil
unless active
 my_limit = num_records
 my_offset = offset
 my_order = { created_at: :desc }
end
@purchases = base_scope
 .where(where_hash)
 .order(my_order)
 .limit(my_limit)
 .offset(my_offset)

As you see the whole if/else block is useless and we shortened it into a smaller one as well. If I'm not wrong, nil values in where, limit, offset and order are simply ignored so if any param is nil it will just not apply that filtering to it instead of returning 0 results. So in practice, your scope will be full by default and minified the more values it is given.