4
\$\begingroup\$

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
\$\endgroup\$
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I'm adding my vote to the use of scopes. If the condition of the bid is an emergent property of the bid itself, then leaving it outside the Bid model will create feature envy. \$\endgroup\$ – Rich Seviora Dec 8 '15 at 19:30
  • \$\begingroup\$ Thanks for your excellent feedback. I do agree that the data modelling is not ideal and we're working on improving it. I did run into one problem current_user.bids.joins(real_property_sale: :real_property_sale_project).where(real_property_sale_projects: { failed_date: nil }) does not return all of the bids excluding ones where the failed date is nil. It only returns bids that have a project and the failed date is nil. You'll notice that the above is slightly different from your suggestion. Any thoughts ? \$\endgroup\$ – Thomas Taylor Dec 9 '15 at 6:11
  • \$\begingroup\$ @ThomasTaylor Ah, right. You may have to resort to more "raw" SQL, both for the join syntax (so it includes all records, not just those that can be be joined with something) and the where-clause. I can't rattle off the exact query here and now, I'm afraid (as I said, I'm a little rusty), but swing by StackOverflow perhaps, and ask there. \$\endgroup\$ – Flambino Dec 9 '15 at 8:04
  • 1
    \$\begingroup\$ I did just that query is: where(agent_id: agent_id) - where(agent_id: agent_id).joins(real_property_sale: :real_property_sale_project).where.not(real_property_sale_projects: { failed_date: nil }) thanks again have learnt a tremendous amount from your answer! \$\endgroup\$ – Thomas Taylor Dec 9 '15 at 8:07
  • \$\begingroup\$ @ThomasTaylor Neat :) \$\endgroup\$ – Flambino Dec 9 '15 at 8:30
2
\$\begingroup\$
@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!

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.