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 9970th person to read this article.