Ruby - Nested IF statement syntax

Question

I'm trying to get this nested if statement to work but my syntax is wrong and I can't figure it out. I have a search box on a Rails 4 app and a sort dropdown. On the search results page, I want to sort product listings based on what the user selects in the sort dropdown. If a user doesn't enter a search term, I want a message to display. Here is my controller code.

If I remove conditions and just display a default sort, search results page displays fine so the error is in the if statement syntax.

def search
    if params[:search].present?

      if params[:sort] == "- Price - Low to High"
        @listings = ...
      elsif params[:sort] == "- Price - High to Low"
        @listings = ...
      elsif params[:sort] == "- New Arrivals"
        @listings = ...
      elsif params[:sort] == "- Random Shuffle"
        @listings = ...
      else
        @listings = ...
      end

    else
      flash[:notice] = "Please enter one or more search terms e.g. blue shirt."
    end

  end

What is the exact behavior of the code? Is the notice always shown? — Undo, Oct 08 '14 at 16:12
There are too many chained `elsif`s. It looks like you could make heavy use of "switch", that exists in Ruby in form of `case`/`when`: http://stackoverflow.com/questions/948135/how-can-i-write-a-switch-statement-in-ruby — D-side, Oct 08 '14 at 16:14
You are checking that :search is present, but then "switching" on :sort. Is this behavior correct? — Jeff Price, Oct 08 '14 at 16:17
As @D-side pointed out this is in essence a switch statement (which are usually considered code-smell in ruby). Maybe identify what you are trying to do and we can help you from there because you can probably convert this to 1 or 2 lines. — engineersmnky, Oct 08 '14 at 16:24
@Undo the notice is shown only when the user clicks on the search button without entering a search term. This is a basic ecommerce app where users are searching for products to buy. The sort dropdown is on the search results page. — Moosa, Oct 08 '14 at 16:30

score 3 · Accepted Answer · answered Oct 08 '14 at 16:25

What you want here is a case statement, a switch from other languages like JavaScript and C:

def search
  if params[:search].present?
    @listings =
      case (params[:sort])
      when  "- Price - Low to High"
        ...
      when "- Price - High to Low"
        ...
      when "- New Arrivals"
        ...
      when "- Random Shuffle"
        ...
      else
        ...
      end
  else
    flash[:notice] = "Please enter one or more search terms e.g. blue shirt."
  end
end

In Ruby, the result of a case statement can be used to assign to a variable, so that eliminates a lot of the @listings = repetition. Same goes for an if.

The things you're comparing against here seem unusually specific. If you've got a drop-down listing that's being used to choose sort order, they should have more succinct values used internally, such as plh to represent "Price - Low to High", or even numerical codes. That means if the phrasing is changed your code still works.

score 0 · Answer 2 · answered Oct 08 '14 at 16:32

I would change your drop down to have values like price asc,price desc, arrival_date asc (not sure about random shuffle) then you could use.

def search
  if params[:search].present?
    sort_listings(params[:sort])
  else
    flash[:notice] = "Please enter one or more search terms e.g. blue shirt."
  end
end

def sort_listings(sort)
  @listings ||= ...
  @listing.order(sort)
end

Ruby - Nested IF statement syntax

2 Answers2