Clean a controller action being explicit about the business rules

December 26, 2017

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.

Weekly tips and tools for Ruby on Rails developers