Show Us Your Production Code #1: Concerns

Michiel Sikkes of Firmhouse responded to my Show Us Your Production Code appeal with a blog post called Production code: I am concerned.

Michiel asked for suggestions and what follows are my ideas on how I would refractor the code (you need to read Michiel's post first for his solution).

Offender

Here is the offending code:

def index 
# Code omitted
if params[:sort] == "product_name"
    if params[:direction] == "desc"
      @products = @products.order("products.name desc")
    else
      @products = @products.order("products.name asc")
    end
  elsif params[:sort] == "shop_code"
    if params[:direction] == "desc"
      @products = @products.order("products.final_article_number_cache desc")
    else
      @products = @products.order("products.final_article_number_cache asc")
    end
  else
    if ENV['SITE_ALIAS'].nil?
      # Sort based on product description for Stofzuigermarkt
      @products = @products.order("products.name")
    else
      @products = @products.order('products.final_article_number_cache')
    end
  end
# Code omitted
end

Ain't that some production code?

Michiel decided to use controller concerns to refactor this code but I don't agree with this decision. What this code does is orders products based on the request's params.

For me that looks like a model scope with some logic because incoming params differ from the model's column names.

My decision is to create a simple scope on the Product model. The method will be somewhat big and to keep our models from getting fat we can futher extract the code into a model concern.

Refactoring

# app/models/concerns/orderable_products.rb
module OrderableProducts
  extend ActiveSupport::Concern

  module ClassMethods
    def ordered(column, direction = nil)
      column = map_column(column)

      order = [column, direction].reject(&:blank?).join(' ')

      order(order)
    end

    private
      def map_column(column)
        mappings = {
          product_name: 'name', 
          shop_code: 'final_article_number_cache'
        }

        column = mappings[column]
        column ||= ENV['SITE_ALIAS'].nil? ? 'name' : 'final_article_number_cache'
      end
  end
end

In your model:

class Product < ActiveRecord::Base
  include OrderableProducts
  # ...
end

And in your controller you can now use ordered as a scope:

@products = Product.upcoming.active.ordered(params[:sort], params[:direction])

As you see, Michiel and I approached this problem differently. And that's awesome because we both can learn something from each other.

If you think you've got a better solution for this problem, let me know. Getting other people's perspective is a great way to become better.

You're the 9868th person to read this article.