How to simplify big conditions

Question

I have five dropdowns, and I need to put conditions on each of them. My code is:

   def search(search, compare, year, rain_fall_type)
    if search == 'All'
      if rain_fall_type == 'All'
        all
      else
        if year == 'All'
          if rain_fall_type == "None"
            where('Sector = ? OR Sector = ? OR Sector = ?', "Primary", 'Secondary', 'Tertiary').order('id')
          else
            # all
             where(Sector: rain_fall_type).order('id')
          end
        else
          if rain_fall_type == "All"
            order("#{year} ")

          elsif rain_fall_type == "None"
            where('Sector = ? OR Sector = ? OR Sector = ?', "Primary", 'Secondary', 'Tertiary').order('id')
           else
            where(Sector: rain_fall_type).order("#{year} ")
           end
        end
        # where(Year: year).order("#{rain_fall_type} ")
      end
    elsif compare != "None"
      if year == 'All'
        where('Sector = ? OR Sector = ?', rain_fall_type, compare).order(:id)
      else
        where('Sector = ? OR Sector = ?', rain_fall_type, compare).order(:id)
      end
    else

      if rain_fall_type == 'All'
        all.order('id')
      else
        if year == 'All'

          if rain_fall_type == "None"
            where('Sector = ? ', search).order('id')
          else

            where('Sector = ? ', rain_fall_type).order('id')
          end
        else

          if rain_fall_type == "None"

            if search == "All"
              where('Sector = ? ', search).order('id')
            else
              where('Sector = ? ', search).order('id')
            end
          else
            # all
            where('Sector = ? ', rain_fall_type).order('id')
          end
        end
      end
    end
  end

It has many if and else. I am trying to minimise the conditions. What can be the best way to shrink this code? Someone suggested that I should use switch case instead. Should I use it? If so, how?

Answer 1

First of all, some of your logic doesn't make sense:

def search(search, compare, year, rain_fall_type)
  if search == 'All'
    if rain_fall_type == 'All'
      all
    else
      # rain_fall_type != 'All'
      if year == 'All'
        if rain_fall_type == "None"
          where('Sector = ? OR Sector = ? OR Sector = ?', "Primary", 'Secondary', 'Tertiary').order('id')
        else
          where(Sector: rain_fall_type).order('id')
        end
      else
        # in rain_fall_type != 'All' branch, so meaningless 'if'
        if rain_fall_type == "All"
          order("#{year} ")
        elsif rain_fall_type == "None"
          where('Sector = ? OR Sector = ? OR Sector = ?', "Primary", 'Secondary', 'Tertiary').order('id')
        else
          where(Sector: rain_fall_type).order("#{year} ")
        end
      end
    end
  elsif compare != "None"
    # both are same, so meaningless 'if'
    if year == 'All'
      where('Sector = ? OR Sector = ?', rain_fall_type, compare).order(:id)
    else
      where('Sector = ? OR Sector = ?', rain_fall_type, compare).order(:id)
    end
  else
    # search != 'All'
    if rain_fall_type == 'All'
      all.order('id')
    else
      if year == 'All'
        if rain_fall_type == "None"
          where('Sector = ? ', search).order('id')
        else
          where('Sector = ? ', rain_fall_type).order('id')
        end
      else
        if rain_fall_type == "None"
          # in search != 'All' branch, so meaningless 'if'
          # AND both are same, so again meaningless 'if'
          if search == "All"
            where('Sector = ? ', search).order('id')
          else
            where('Sector = ? ', search).order('id')
          end
        else
          where('Sector = ? ', rain_fall_type).order('id')
        end
      end
    end
  end
end

There's more like that and I won't point it all out because we're throwing all that if stuff out, anyway.

Ultimately, we're going to defer the querying to the end of the method, like this:

def search(search, compare, year, rain_fall_type)

  ...

  @query = all 
  @query = @query.where(Sector: @sectors) if @sectors
  @query = @query.order(@order) if @order
  @query

end

That way, you take all of your where and order statements, and do them only once at the end. That saves a lot of typing right there. See the comment from muistooshort for why (Sector: @sectors) works.

So, the trick is setting @sectors and @order. First, I'm going to assign the input variables to instance variables because I like it like that (and to avoid confusion between the variable @search and the method search):

def search(search, compare, year, rain_fall_type)
  @search, @compare, @year, @rain_fall_type = search, compare, year, rain_fall_type

  ...

  @query = all 
  @query = @query.where(Sector: @sectors) if @sectors
  @query = @query.order(@order) if @order
  @query
end

Now, this answer is going on too long already, so I won't drag you through all the gorey details. But, adding in a couple of helper methods (sectors_to_use, and order_to_use) and substituting them in for @sectors and @order, you basically end up with this:

def search(search, compare, year, rain_fall_type)
  @search, @compare, @year, @rain_fall_type = search, compare, year, rain_fall_type
  @query = all 
  @query = @query.where(Sector: sectors_to_use) if sectors_to_use
  @query = @query.order(order_to_use) if order_to_use
  @query
end

private 

def sectors_to_use
  return [@rain_fall_type, @compare] if @search != 'All' && @compare != 'None'
  unless @rain_fall_type == 'All'
    if @rain_fall_type == 'None'
      @search == 'All' ? ['Primary', 'Secondary', 'Tertiary'] : [@search]
    else
      [@rain_fall_type]
    end  
  end
end

def order_to_use
  return nil if (@search == 'All') && (@rain_fall_type == 'All')
  return @year if (@search == 'All') && !(@year == 'All')
  return :id
end

That's less than half the lines of code, over a thousand fewer characters, and a whole lot fewer ifs.

Answer 2

This is probably the best explanation of how you should set this up.

class Product < ActiveRecord::Base
  # custom_scope_1
  scope :status, -> (status) { where status: status }
  # custom_scope_2
  scope :location, -> (location_id) { where location_id: location_id }
  # custom_scope_3
  scope :search, -> (name) { where("name like ?", "#{name}%")}
end

def index
   @products = Product.where(nil) # creates an anonymous scope
   @products = @products.status(params[:status]) if params[:status].present?
   @products = @products.location(params[:location]) if params[:location].present?
   @products = @products.search(params[:search]) if params[:search].present?
end

This can be cleaned up further by...

def index
  @products = Product.where(nil)
  filtering_params(params).each do |key, value|
    @products = @products.public_send(key, value) if value.present?
  end
end

private

# A list of the param names that can be used for filtering the Products
def filtering_params(params)
  params.slice(:status, :location, :search)
end

This method uses ruby meta-programming to loop through parameters and dynamically call predefined scopes on a model

You can move this code into a module and include it into any model that supports filtering

app/models/concerns/filterable.rb

module Filterable
  extend ActiveSupport::Concern

  module ClassMethods
    def filter(filtering_params)
      results = self.where(nil)
      filtering_params.each do |key, value|
        results = results.public_send(key, value) if value.present?
      end
      results
    end
  end
end

app/models/product.rb

class Product
  include Filterable
  ...
end

app/controllers/product_controller.rb

def index
  @products = Product.filter(params.slice(:status, :location, :search))
end

You now have filtering and searching of your models with one line in the controller and one line in the model

Answer 3

You can use a guard statement which is basically return something if some_condition?. This only doable in specific scenarios (where one of the condition is executing a single statement:

Bad example:

if condition?
  do_something
else
  do_something_else
end

This could be written as:

return do_something if condition?
do_something_else

This will give you less code branching.

---

Also, another recommendation is to call another method with more conditions instead of nesting conditions in one single shot.

Bad example:

if condition?
  if condition_two?
    do_something_two
  else
    do_something
  end
else
  do_something_else
end

This could be written as:

if condition?
  call_another_method 
else
  do_something_else
end

def call_another_method
  if condition_two?
    do_something_two
  else
    do_something
  end
end

---

An example from your code could be:

if rain_fall_type == 'All'
  all
else
  if year == 'All'
    if rain_fall_type == "None"
      where('Sector = ? OR Sector = ? OR Sector = ?', "Primary", 'Secondary', 'Tertiary').order('id')
    else
      # all
        where(Sector: rain_fall_type).order('id')
    end
  else
    if rain_fall_type == "All"
      order("#{year} ")

    elsif rain_fall_type == "None"
      where('Sector = ? OR Sector = ? OR Sector = ?', "Primary", 'Secondary', 'Tertiary').order('id')
      else
      where(Sector: rain_fall_type).order("#{year} ")
      end
  end
end

That could be converted to:

return all if rain_fall_type == 'All'
if year == 'All'
  return where('Sector = ? OR Sector = ? OR Sector = ?', "Primary", 'Secondary', 'Tertiary').order('id') if rain_fall_type == "None"
  where(Sector: rain_fall_type).order('id')
else
  return order("#{year} ") if rain_fall_type == "All"
  return where('Sector = ? OR Sector = ? OR Sector = ?', "Primary", 'Secondary', 'Tertiary').order('id') if rain_fall_type == "None"
  where(Sector: rain_fall_type).order("#{year} ")
end

I hope this could help :)

NOTE: This is to answer the original question of How to simplify big conditions?. But the original post is not following Rails/Ruby way of doing search and filters and not making a good use of scopes.