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.