Imagine that you have the next controller action that works, but when you see it you feel that you can make it cleaner.

def index
  if params[:term]
    @hotels = Hotel.where(published: true).search(params[:term]).order("name ASC")
  elsif params[:hotel_type]
    if params[:stars]
      @hotels = Hotel.where(published: true).where(hotel_type_id: params[:hotel_type].to_i).where(stars: params[:stars]).order("name ASC")
    else
      @hotels = Hotel.where(published: true).where(hotel_type_id: params[:hotel_type].to_i).order("name ASC")
    end
  else
    @hotels = Hotel.where(published: true).order("name ASC")
  end
end

There are different ways to try to refactor this code, but now I want to talk about how you can clean your code trying to be very explicit about the business rules that the code is implementing.

1- First lets try to find the specification

Watching the code I can find the next rules:

  • Always show just the published hotels.
  • Always show the hotels ordered by name.
  • If a “term” is present “search” the hotels with that term and don’t take in account other parameters.
  • If the “hotel type” param is given filter the hotels with that type.
  • If the “stars” param is given filter the hotels with that number of starts.

Now that we have the specification we can…

2- Extract functions that can describe those rules

One implementation for this rules could be:

def index
  if params[:term]
    @hotels = Hotel.find_published_ordered_by_name_searching_for(params[:term])
  else
    query = {}

    if params[:hotel_type]
      query.merge(hotel_type_id: params[:hotel_type])
    end

    if params[:stars]
      query.merge(stars: params[:stars])
    end

    @hotels = Hotel.find_published_ordered_by_name_with_query(query)
  end
end

class Hotel < ApplicationRecord
  def self.find_published_ordered_by_name_searching_for(term)
    where(published: true).order(name: :asc).search(term)
  end

  def self.find_published_ordered_by_name_with_query(attrs)
    where(published: true).order(name: :asc).where(attrs.select { |_k, v| v.present? })
  end
end

Here I am trying to be very clear about the two branches that can take our code if the user provides a “term”. Also I am trying to be very explicit saying that we are always looking for “published hotels ordered by name”.

Now that the rules are a little more clear we can…

3- Change little implementation details that can make the code cleaner

For example we can change the param key hotel_type to be hotel_type_id in order to be able to remove the if statements of the second branch. And also we can extract some helper methods in the Hotel class.

def index
  @hotels =
    if params[:term]
      Hotel.find_published_ordered_by_name_searching_for(params[:term])
    else
      Hotel.find_published_ordered_by_name_with_query(params.slice(:hotel_type_id, :stars))
    end
end

class Hotel < ApplicationRecord
  scope :published, -> { where(published: true) }
  scope :by_name, -> { order(name: :asc) }
  scope :search, -> { ... }

  def self.find_published_ordered_by_name_searching_for(term)
    published.by_name.search(term)
  end

  def self.find_published_ordered_by_name_with_query(attrs)
    published.by_name.where(attrs.select { |_k, v| v.present? })
  end
end

Conclusion

Sometimes we try to make the code more “beautiful” but I think is more important to try to express the business rules in the clearest way possible.