Build Dumb, Refactor Smart: How to Massage Problems Out of Rails Code

View all articles
Daniel Lewis has been a professional Ruby on Rails developer for over 4 years, working on about a dozen high-traffic web applications, many of them through Toptal.

Sometimes, clients give us feature requests that we really don’t like. It’s not that we don’t like our clients—we love our clients. It’s not that we don’t like the feature—most client-requested features are aligned perfectly with their business goals and income.

Sometimes, we don’t like a feature request because the easiest way to solve it is to write bad code, and we don’t have an elegant solution on the top of our heads. This will throw many of us on fruitless searches through RubyToolbox, GitHub, developer blogs, and StackOverflow looking for a gem or plugin or example code that will make us feel better about ourselves.

Sometimes, we don't like a feature request because the easiest way to solve it is to write bad code.

Well, I’m here to tell you: it’s okay to write bad code. Sometimes, bad code is easier to refactor into beautiful code than a poorly thought-out solution implemented under a time-crunch.

This is the process I like to follow when refactoring my horrible band-aid solutions:

For an alternative perspective, here’s the Git commit log for a feature that’s been refactored step-by-step:

Let’s see how it’s done.

The Views

Step 1. Start in the Views

Say we are beginning a ticket for a new feature. The client tells us: “Visitors should be able to view a list of active Projects on the welcome page.”

This ticket requires a visible change, so a reasonable place to start work would be in the Views. The problem is straightforward and one that we’ve all been trained to solve multiple times. I’m going to solve it The Wrong Way and demonstrate how to refactor my solution into it’s appropriate areas. Solving a problem The Wrong Way can help us get over the hump of not knowing the right solution.

To begin, assume we have a model called Project with a boolean attribute called active. We want to get a list of all Projects where active is equal to true, so we can use Project.where(active: true), and loop over it with an each block.

app/views/pages/welcome.haml:

%ul.projects
  - Project.where(active: true).each do |project|
    %li.project= link_to project_path(project), project.name

I know what you’re saying: “That would never pass a code review” or “My client would surely fire me for this.” Yes, this solution breaks the Model-View-Controller separation of concerns, it might result in stray database calls that are difficult to trace, and it may become difficult to maintain in the future. But consider the value of doing it The Wrong Way:

  1. You can get this change on staging in under 15 minutes.
  2. If left in, this block is easy to cache.
  3. Fixing this Rails faux-pas is straightforward (could be given to a junior developer).

Step 2. Partials

After doing it The Wrong Way, I feel bad about myself and want to isolate my bad code. If this change were clearly only a concern of the View layer, I could refactor my shame into a partial.

app/views/pages/welcome.haml:
= render :partial => 'shared/projects_list'

app/views/shared/projects_list.haml:
%ul.projects
  - Project.where(active: true).each do |project|
    %li.project= link_to project_path(project), project.name

That’s a little better. Clearly, we are still making the mistake of a Model query in a View, but at least when a maintainer comes in later and sees my horrible partial, they will have a straightforward way of tackling that problem in particular. Fixing something obviously dumb is always easier than fixing a poorly-implemented, buggy abstraction.

Fixing something obviously dumb is always easier than fixing a poorly-implemented, buggy abstraction.

Step 3. Helpers

Helpers in Rails are a way of creating a DSL (Domain Specific Language) for a section of your Views. You have to rewrite your code using content_tag’s instead of haml or HTML, but you get the benefit of being allowed to manipulate data structures without having other developers glare at you for 15 lines of non-printing View code.

If I were to use helpers here, I would probably refactor out the li tag.

app/views/shared/projects_list.haml:
%ul.projects
  - Project.where(active: true).each do |project|
    = project_list_item(project)

app/helpers/projects_helper.rb:
  def project_list_item(project)
    content_tag(:li, :class => 'project') do
      link_to project_path(project), project.name
    end
  end

The Controllers

Step 4. Move It to the Controllers

Maybe your awful code isn’t just a View concern. If your code still smells, look for queries you can transition from the Views to the Controllers.

app/views/shared/projects_list.haml:
%ul.projects
  - @projects_list.each do |project|
    = project_list_item(project)

app/controllers/pages_controller.rb:
  def welcome
    @projects = Project.where(active: true)
  end

Step 5. Controller Filters

The most obvious reason to move code into a Controller before_filter or after_filter is for code that you’ve duplicated in multiple Controller actions. You can also move code into a Controller filter if you want to separate the purpose of the Controller action from the requirements of your views.

app/controllers/pages_controller.rb:
  before_filter :projects_list

  def welcome
  end

  def projects_list
    @projects = Project.where(active:true)
  end

Step 6. Application Controller

Presume that you need your code to show up on every page, or you want to make Controller helper functions available to all controllers, you can move your function into the ApplicationController. If the changes are global, you may want to modify your application layout as well.

app/controllers/pages_controller.rb:
  def welcome
  end

app/views/layouts/application.haml:
    %ul.projects
      - projects_list.each do |project|
        = project_list_item(project)

app/controllers/application_controller.rb:
  before_filter :projects_list

  def projects_list
    @projects = Project.where(active: true)
  end

The Models

Step 7. Refactoring to the Model

As the MVC motto goes: Fat Model, Skinny Controllers, and Dumb Views. We’re expected to refactor everything we can into the Model, and it’s true that most complex functionality will eventually become model associations and model methods. We should always avoid doing formatting/view things in the Model, but transforming data into other types of data is permissible. In this case, the best thing to refactor into the Model would be the where(active: true) clause, which we can turn into a scope. Using a scope is valuable not only because it makes the call look prettier, but if we ever decided to add an attribute like delete or outdated, we can modify this scope instead of hunting down all of our where clauses.

app/controllers/application_controller.rb:
  before_filter :projects_list

  def projects_list
    @projects = Project.active
  end

app/models/project.rb:
  scope :active, where(active: true)

Step 8. Model Filters

We don’t have a particular use for a Model’s before_save or after_save filters in this case, but the next step I usually take is to move functionality from Controllers and Model methods into Model filters.

Say we had another attribute, num_views. If num_views > 50, the Project becomes inactive. We could solve this problem in the View, but making database changes in a View is inappropriate. We could solve it in the Controller, but our Controllers should be as thin as possible! We can solve it easily in the Model.

app/models/project.rb:
  before_save :deactivate_if_over_num_views

  def deactivate_if_over_num_views
    if num_views > 50
      self.active = false
    fi
  end

Note: you should avoid calling self.save in a Model filter, as this causes recursive saving events, and the database-manipulation layer of your application should be the Controller anyway.

Step 9. Libraries

Occasionally, your feature is large enough that it could warrant it’s own library. You may want to move it into a library file because it’s reused in a lot of places, or it’s large enough that you’d like to do development on it separately.

It’s fine to store library files in the lib/ directory, but as they grow, you can transfer them into a real RubyGem! A major advantage of moving your code into a library is that you can test the library separately from your model.

Anyway, in the case of a Project List, we could justify moving the scope :active call from the Project model into a library file, and bring it back into Ruby:

app/models/project.rb:
class Project < ActiveRecord::Base
  include Activeable

  before_filter :deactivate_if_over_num_views
end

lib/activeable.rb:
module Activeable
  def self.included(k)
    k.scope :active, k.where(active: true)
  end

  def deactivate_if_over_num_views
    if num_views > 50
      self.active = false
    end
  end
end

Note: the self.included method is called when a Rails Model class is loaded and passes in the class scope as the variable k.

Conclusion

In under 15 minutes, we’ve implemented a solution and put it up on staging for user testing, ready to be accepted into the feature set or removed. By the end of the refactoring process, we have a piece of code that lays out a framework for implementing list-able, activate-able items across multiple Models that would pass even the most stringent review process.

In your own refactoring process, feel free to skip a few steps down the pipeline if you are confident in doing so (e.g., jump from View to Controller, or Controller to Model). Just keep in mind that the flow of code is from View to Model.

Don’t be afraid to look stupid. What separates modern languages from old CGI template rendering applications isn’t that we do everything The Right Way every time—it’s that we take the time to refactor, reuse, and share our efforts.

Hiring? Meet the Top 10 Ruby on Rails Developers for Hire in October 2014
Like what you're reading?
Get the latest updates first.
Don't miss out.
Get the latest updates first.

Comments

Carlos Alexandro Becker
Hi Daniel, nice article! Following your example, let's suppose that the "project_list_item" is actually much bigger and complex (i.e.: lots of elements inside it, like a table row with multiple columns), what you usually do? Recently I get in a situation like that, and put the row in a html.erb file, and just rendered it in the helper method... but it doesn't feel right to me, so, I would like to hear your opinion. Cheers.
twopoint718
If the view representation for the "projects" got too complex, I'd reach for decorators. The idea is you do something like: @projects = Project.all.decorate and then have a class which implements the view logic for "Project". Then you can be really declarative in your views and also not clutter your models with view logic. See https://github.com/drapergem/draper for good examples.
Carlos Alexandro Becker
Gotcha. Thank you very much!
Sasha Koss
I have to say that project_list_item is overhead, there is no reason to hide such simple HTML snippet into helper. Helpers should be used to hide logic from markup, but here is opposite situation. Better to move self.active = false into deactivate method ;-).
Paweł Gościcki
Fully agree! Having a helper to hide such simple HTML does not really help.
Dreamr OKelly
I usually don't like concerns as I like to get away from dependencies, but in this case your activateable should be a concern. My reasoning is this; it is talking about a scope, something very arel dependent. For this I would do app/models/concerns/activateable.rb Other than that nit-picky shit at the end, wonderful approach to refactoring in a sensible manner.
comments powered by Disqus
Subscribe
Free email updates
Get the latest content first.
Trending articles
Relevant technologies
Toptal Authors
JavaScript Developer
iOS developer
Software Engineer
Alvaro Oliveira
Director of Engineering @ Toptal
Ruby on Rails Engineer