Rails conditional sorting based on params

Question

The following code sorts the current users bid model items based on a param condition :bid_status. It starts to get messy when checking if the project space (:real_property_sale_project) :failed_date is .present? or .nil? to exclude certain items from the returned results. How can I improve this?

def index
  @bids = current_user.bids.select { |bid| bid.real_property_sale.try(:real_property_sale_project).try(:failed_date).nil? }

  if params[:bid_status] == 'in_progress'
    @bids = @bids.select { |bid| bid.real_property_sale.status == 'published' }     
  elsif params[:bid_status] == 'awarded'
    @bids = @bids.select { |bid| bid.status == 'accepted' && bid.real_property_sale.try(:real_property_sale_project).try(:failed_date).nil? }
  elsif params[:bid_status] == 'lost'
    @bids = @bids.select { |bid| bid.status == 'rejected' }
  elsif params[:bid_status] == 'history'
    @bids = current_user.bids.select { |bid| bid.status == 'accepted' && bid.real_property_sale.try(:real_property_sale_project).try(:failed_date).present? }
  end

  @bids = @bids.paginate(:page => params[:page], :per_page => 9)
end

Answer 1

Don't use select - that requires loading all records, and then filtering/sorting them in Ruby. But sorting and filtering is what databases and SQL is for.

For the first part, something like this should work (my Rails query-fu is rusty, so no guarantees):

current_user.bids
  .joins(real_property_sale: :real_property_sale_projects)
  .where(real_property_sale_project: { failed_date: nil })

On a side note, it seems like your models are overly complicated. They share the same prefix ("real property sale") which would seem to suggest they're been split up in a strange way. Not saying that's the case, but it does seem... off, somehow.

Whatever your app is doing, I'd wager that a user's bids are pretty important. So it seems strange that it'd take this much work to load them. A "bid status" should probably be front-and-center in the models; not a complex derived value, requiring loading not just 1st order but 2nd order associations. Just seems suspect to me.

Anyway, that code should probably be hidden away in a scope on the Bid model, so you can just say:

current_user.bids.active

or whatever name makes sense for this particular collection of bids.

Also, in your current code it seems that for the "awarded" value, you redo the same filtering you've already done once (checking failed_date).

Anyway, I imagine something like this:

@bids = case params[:bid_status]
        when 'in_progress'
          current_user.bids.active.where(real_property_sale: { status: "published" })
        when 'awarded'
          current_user.bids.active.where(status: "accepted")
        when 'lost'
          current_user.bids.active.where(status: "rejected")
        when 'history'
          current_user.bids
            .joins(real_property_sale: :real_property_sale_projects)
            .where("real_property_sale_project.failed_date IS NOT NULL")
        else
          current_user.bids.active
        end

Again, I can't guarantee it'll work as-is, but it illustrates the structure.

Note also that scopes can be chained, so you could conceivably make scopes for each of the filters, and do things like bids.active.accepted, bids.active.rejected, etc..

If you have named scopes for each filter, you could even do stuff like:

current_user.bids.active.send(params[:bid_status])

provided that a) you're sure it's safe to send the param (i.e. it's been whitelisted), and b) you use the same param values as the scope names.

Right now, it's a little odd that none of your param values actually match anything in the data models; "awarded" really means "accepted", "lost" really means "rejected", "in progress" actually means "published", etc..

Again, it seems like your data modelling is little off.

Answer 2

@bids = current_user.bids.select { |bid| bid.real_property_sale.try(:real_property_sale_project).try(:failed_date).nil? }

if ...
  @bids = @bids.select { |bid| ... }     
elsif ...
  @bids = @bids.select { |bid| ... }
elsif ...
  @bids = @bids.select { |bid| ... }
elsif ...
  @bids = current_user.bids.select { |bid| bid.status == 'accepted' && bid.real_property_sale.try(:real_property_sale_project).try(:failed_date).present? }
end

The first line filters the bids according to some criterion, but the result is only used in the first 3 cases (which further filter the bids). The last case ignores it, which is confusing because the first line runs in every case.

In order to simplify the code, you must first make it more consistent. This is achieved by putting this condition in every case that uses it: (temporarily making the code longer)

@bids = current_user.bids

if ...
  @bids = @bids.select { |bid| bid.real_property_sale.try(:real_property_sale_project).try(:failed_date).nil? && ... }     
elsif ...
  @bids = @bids.select { |bid| bid.real_property_sale.try(:real_property_sale_project).try(:failed_date).nil? && ... }
elsif ...
  @bids = @bids.select { |bid| bid.real_property_sale.try(:real_property_sale_project).try(:failed_date).nil? && ... }
elsif ...
  @bids = @bids.select { |bid| bid.real_property_sale.try(:real_property_sale_project).try(:failed_date).present? && bid.status == 'accepted' }
end

Now all cases have the same form: @bids.select { ... }. This means you can replace it with a single @bids.select statement and put the conditionals inside it:

@bids = current_user.bids.select do |bid|
  if ...
    bid.real_property_sale.try(:real_property_sale_project).try(:failed_date).nil? && ...
  elsif ...
    bid.real_property_sale.try(:real_property_sale_project).try(:failed_date).nil? && ...
  elsif ...
    bid.real_property_sale.try(:real_property_sale_project).try(:failed_date).nil? && ...
  elsif ...
    bid.real_property_sale.try(:real_property_sale_project).try(:failed_date).present? && ...
  end
end

Now you can extract local variables to eliminate duplication. You can also use a case statement instead of the if-elsif chain:

def index
  bids = current_user.bids.select do |bid|
    has_failed_date = bid.real_property_sale.try(:real_property_sale_project).try(:failed_date).present?
    status = bid.status

    case params[:bid_status]
    when 'in_progress'
      !has_failed_date && bid.real_property_sale.status == 'published'
    when 'awarded'
      !has_failed_date && status == 'accepted'
    when 'lost'
      !has_failed_date && status == 'rejected'
    when 'history'
      has_failed_date && status == 'accepted'
    end
  end

  @bids = bids.paginate(:page => params[:page], :per_page => 9)
end

(In the first line I changed @bids to a local variable bids. There's no need to set @bids in the first line.)

---

Note that this solution might be slower, as params[:bid_status] is rechecked for every bid. If this proves to be a problem, you can solve it by checking params[:bid_status] once before the call to select and creating a lambda expression to be passed to select:

filter_criterion =
  case params[:bid_status]
  when 'in_progress'
    -> (bid) { !bid_has_failed_date?(bid) && bid.real_property_sale.status == 'published' }
  when 'awarded'
    -> (bid) { !bid_has_failed_date?(bid) && bid.status == 'accepted' }
  when 'lost'
    -> (bid) { !bid_has_failed_date?(bid) && bid.status == 'rejected' }
  when 'history'
    -> (bid) { bid_has_failed_date?(bid) && bid.status == 'accepted' }
  end

bids = current_user.bids.select(&filter_criterion)

This requires adding a private bid_has_failed_date?(bid) method to your controller.

Actually, because this method is about bids, it makes more sense to put it in the Bid class instead, and then write bid.has_failed_date? in the controller instead of bid_has_failed_date?(bid). Even you choose the first solution, it's probably a good idea to introduce a Bid#has_failed_date? method.

Good luck!