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

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 send many of us Rails developers 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.

Refactor Ruby on Rails Code

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

This is the Rails refactoring process I like to follow when massaging problems out of 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:

And here’s another interesting article about large-scale refactoring from a colleague in the Toptal network.

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 problem 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 Rails code problem in particular. 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 this Ruby on Rails refactoring tutorial we’ve taken under 15 minutes and 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 Rails 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.