Refactoring Legacy Rails Controllers

Development

Ruby on Rails controllers are like the bouncers of a nightclub. No identification at a club? You aren’t getting in. Without the proper clothes, you can expect to be turned away. Oh, you wanna say something slick? You’re definitely not getting in, and you might be getting a beatdown on your way out.

Controllers are the bouncers of the Rails software stack. You aren’t logged in? See ya. You aren’t authorized to view that resource? Goodbye. Oh, you’re trying to change an attribute that you don’t have access to? You’re gone, and your account is probably getting banned too.

Controllers Rule Everything Around Me

They say the eyes are the window to the soul. Controllers are the window to the soul of your application data. All requests start at the router and ultimately must pass through your controllers to get access to the data and the views.

rails-mvc

Controllers wear many hats. They are responsible for:

  • Authenticating users
  • Authorizing user access to resources
  • Sanitizing data input from the user
  • Loading data from the models
  • Rendering the view

Controllers wield massive power in a Rails application. Because controllers are so omnipotent, it’s incredibly easy for a novice programmer to bloat a controller with business logic.

Controllers Are Difficult to Unit Test

By their very nature, controllers integrate and connect multiple areas of your application. This makes controllers difficult to isolate and unit test. Any time you find yourself in a controller action, you began your journey in the router. Once you’re inside the controller, a model is typically loaded and a view is rendered. The most basic Rails controllers do all of these things.

class PostsController < ApplicationController
  def new
    @post = Post.new
  end

  def create
    @post = Post.new(post_params)
    if @post.save
      redirect_to @post
    else
      render :new
    end
  end

  private

  def post_params
    params.require(:post).permit(:title, :body, :author)
  end
end

Controllers Breed Bad Code

Due to their massive responsibility, controllers are highly susceptible to code bloat. They are difficult to test because they integrate multiple areas of the application. This makes controllers the ideal breeding ground for untested, spaghetti code.

The most basic controller #create actions require at least six lines of code. We must be vigilant about keeping our controller methods small and passing business logic responsibility down to the models or plain Ruby objects. When we force business logic down and out of our controllers, we gain the benefit of being able to unit test application logic. Our code consequently becomes easier to understand.

Legacy Ruby code is a nightmare

Legacy code is just the fancy way of saying untested code. Untested Ruby code is a nightmare. Ruby offers no variable type safety. As Ruby is an interpreted language, you get no compiler safety. The entire Ruby standard library can be overwritten with monkey patches. Without tests, you have no assurances that your code changes won’t break the entire application.

The first step in the battle against bloated controllers will always be to get the legacy code under test.

As we’ve already discussed, it’s incredibly difficult to test business logic that lives in the controller. Strap up, because that’s what we have to do. I’ll warn you in advance: Your controller tests will end up very large. Don’t worry, because it’s only a temporary condition. Here’s our blueprint for wrangling legacy controller code:

  1. Wrap controller code with pending tests to gain understanding.
  2. Write out the tests. Let test failures guide you to green tests.
  3. When you have full coverage, refactor aggressively down to the models or extract plain Ruby objects.
  4. Refactor your controller integration tests into unit tests.

Let’s Refactor!

This code comes from a real project that I’ve worked on. I’ve greatly simplified the controller action to its most essential pieces. It was originally over 100 lines! The refactoring concepts discussed in this article scale all the way up to the toughest refactors.

class TimeEntriesController < ApplicationController
  before_filter :authenticate_user!

  def send_entries
    if params[:start_date].present?
      @start_date = Date.parse(params[:start_date])
    else
      @start_date = Time.zone.today
    end

    if params[:end_date].present?
      @end_date = Date.parse(params[:end_date])
    else
      @end_date = Time.zone.today
    end

    @time_entries = TimeEntry.order("actual_start_time DESC")
                             .where("(actual_hours IS NOT NULL OR bill_amount IS NOT NULL) AND actual_start_time >= ? AND actual_start_time <= ?", @start_date.beginning_of_day, @end_date.end_of_day)

    UserMailer.send_time_entries(current_user, @time_entries).deliver_now
    flash[:success] = "Time Entries were successfully sent to you."
    redirect_to root_path
  end
end

The controller is using #deliver_now, which sends the time entries email within the web request. This can take up to five seconds. Our goal is to port the email send-out to a background job.

In Rails 4.2+, you can background the sending of emails with the #deliver_later method. Our task is to change the deliver_now over to a deliver_later.

Let’s just change the method over right now. Maybe we don’t need to refactor at all.

UserMailer.send_time_entries(current_user, @time_entries).deliver_later

#deliver_later queues up the email with Rails’ Active Job library. Active Job serializes Active Record objects down to simple strings. When the job is activated later, Active Job can just read the attributes of your object from the string.

The problem with this code is that Active Job does not support serializing collections of Active Record objects. The @time_entries instance variable is populated with a collection of TimeEntry objects.

  @time_entries = TimeEntry.order("actual_start_time DESC")
                           .where("(actual_hours IS NOT NULL OR bill_amount IS NOT NULL) AND actual_start_time >= ? AND actual_start_time <= ?", @start_date.beginning_of_day, @end_date.end_of_day)

When you try to serialize an Active Record collection, you get the following error:

active_job_error

The error is telling us that we need to perform the TimeEntry query inside of the background job, instead of passing @time_entries to the mailer. That means we need all the information to recreate the query inside of the mailer, i.e., the parameters from the controller.

We could make this code work right now by passing the parameters down to the mailer method and performing all the query logic inside of the mailer. Here’s the mailer before we make our changes:

class UserMailer < ApplicationMailer
  default from: 'no-reply@example.com'

  def send_time_entries(user, time_entries)
    @recipient = user
    @time_entries = time_entries
    mail(to: @user.email, subject: 'Your Time Entries Summary')
  end
end

Here’s how the mailer method expands if we choose to not refactor:

class UserMailer < ApplicationMailer
  default from: 'no-reply@example.com'

  def send_time_entries(user, params)
    @recipient = user

    if params[:start_date].present?
      @start_date = Date.parse(params[:start_date])
    else
      @start_date = Time.zone.today
    end

    if params[:end_date].present?
      @end_date = Date.parse(params[:end_date])
    else
      @end_date = Time.zone.today
    end

    @time_entries = TimeEntry.order("actual_start_time DESC")
                             .where("(actual_hours IS NOT NULL OR bill_amount IS NOT NULL) AND actual_start_time >= ? AND actual_start_time <= ?", start_date.beginning_of_day, end_date.end_of_day)

    mail(to: @user.email, subject: 'Your Time Entries Summary')
  end
end

With the above code transfer, we’ve achieved our task of being able to background the email. Since you are just passing a single Active Record object (user) and a Ruby hash (params), Active Job won’t complain about being unable to serialize an Active Record collection.

The question we should be asking is, “Why don’t we just do that and call it a day?”

The answer is because the mailer shouldn’t need to know how to construct a query. If we ever need to change how the query works or if we need to change the TimeEntry model attributes, we have to go into a mailer class and change email code.

Doesn’t that sound ridiculous?

Because the query logic is embedded inside of the mailer, we have no way to reuse that query code. We can’t call UserMailer#send_time_entries from other areas in our codebase because it’s directly coupled with sending out an email.

If we ever need to make that same query in a view or anywhere else, we’d have to copy and paste the code. If the query needs an update, it’ll need to be updated in multiple places. This is a disastrous approach to software development. Refactoring the query is our best option.

hbspt.cta.load(1169977, ‘11903a5d-dfb4-42f2-9dea-9a60171225ca’);

The Discovery Phase

Wrap controller code with pending tests to gain understanding.

We have to surround the controller action with a blanket of tests so that we can refactor safely.

Since it’s code that we don’t understand, the goal of our pending tests is to document all of the paths through the code. Code paths are created by conditionals.

The most common way to fork a path through code is with conditionals: if, unless, else, elsif, ? :(ternary operator), and case. Conditional expressions can be combined with || and &&.

Any time you come across a conditional, there needs to be a test. Any time you come across an area where a conditional is combined, you need an additional test.

Essentially, our tests are going to document all of the conditionals in our controller method. Start at the top of the method and work your way down through the conditionals.

This is the most critical step of our refactoring process. All of the other steps depend on nailing the Discovery Phase.

In our controller, we have two conditionals, two if-else statements. There are two paths through an if-else statement, so we’ll need four tests to document the if-else code paths.

class TimeEntriesController < ApplicationController
  before_filter :authenticate_user!

  def send_entries
    if params[:start_date].present?
      @start_date = Date.parse(params[:start_date])
    else
      @start_date = Time.zone.today
    end

    if params[:end_date].present?
      @end_date = Date.parse(params[:end_date])
    else
      @end_date = Time.zone.today
    end

    @time_entries = TimeEntry.order("actual_start_time DESC")
                             .where("(actual_hours IS NOT NULL OR bill_amount IS NOT NULL) AND actual_start_time >= ? AND actual_start_time <= ?", @start_date.beginning_of_day, @end_date.end_of_day)

    UserMailer.send_time_entries(current_user, @time_entries).deliver_now
    flash[:success] = "Time Entries were successfully sent to you."
    redirect_to root_path
  end
end

The first if statement is checking if the :start_date param is present. Our pending tests start off looking like this:

  context 'start date is present'
  context 'start date is not present'

Despite whatever happens in the first if statement, the code will always pass through the second if statement. The second if statement checks the presence of the :end_date param. Under each :start_date context, we’ll need a context for the two :end_date conditions, present and not present.

context 'start date is present'
  context 'end date is present'
  context 'end date is not present'

context 'start date is not present'
  context 'end date is present'
  context 'end date is not present'

We’ve accounted for the conditionals in our controller. Regardless of our path through the conditionals, three things always happen in our controller action:

  • An email is always sent.
  • A success message is always flashed to the user.
  • The user always gets redirected to the root_path.

Let’s add pending tests for these three events.

it 'delivers an email'
it 'flashes a success message'
it 'redirects to root path'

context 'start date is present'
  context 'end date is present'
  context 'end date is not present'

context 'start date is not present'
  context 'end date is present'
  context 'end date is not present'

Finally, we need to document what we’re expecting the code to do when the :end_date and :start_date params are present/not present.

Looking at the controller, the date parameters control which time entries get returned from the TimeEntry query.

@time_entries = TimeEntry.order("actual_start_time DESC")
                         .where("(actual_hours IS NOT NULL OR bill_amount IS NOT NULL) AND actual_start_time >= ? AND actual_start_time <= ?", @start_date.beginning_of_day, @end_date.end_of_day)

If we have a :start_date and we have an :end_date, then we’ll get time entries that started between those two parameters. We’ll add a pending test for that.

context 'start date is present'
  context 'end date is present'
    it 'sends entries between start date and end date'

We’ve gone line by line and discovered what the code is trying to accomplish with the date parameters. Let’s add the tests for the rest of the contexts.

context 'start date is present'
  context 'end date is present'
    it 'sends entries between start date and end date'

  context 'end date is not present'
    it 'sends entries between start date and today'

context 'start date is not present'
  context 'end date is present'
    it 'sends entries between today and end date'

  context 'end date is not present'
    it 'sends entries only from today'

Congratulations, we’ve now documented everything that happens in the controller action!

describe TimeEntriesController do
  describe '#send_entries' do
    it 'delivers an email'
    it 'flashes a success message'
    it 'redirects to root path'

    context 'start date is present'
      context 'end date is present'
        it 'sends entries between start date and end date'

      context 'end date is not present'
        it 'sends entries between start date and today'

    context 'start date is not present'
      context 'end date is present'
        it 'sends entries between today and end date'

      context 'end date is not present'
        it 'sends entries only from today'
  end
end

By documenting what we expect from the code, we’ve gained a maximum understanding of what the previously foreign code is supposed to accomplish. Now we can start filling out the tests!

The Bootstrapping Phase

Write out the tests. Let test failures guide you to green tests.

We are clueless about this codebase. To get these tests running, we’ll need to create objects that we know nothing about. Errors will be our best friend in the bootstrapping phase. They will guide us to creating the objects we need to set up our tests.

We’ll start with the most fundamental test, the delivering of the email.

describe '#send_entries' do
  it 'delivers an email' do
    time_entry = TimeEntry.create!(actual_start_time: Time.current)
    post :send_invoice

    last_email = ActionMailer::Base.deliveries.last
    expect(last_email).to have_content time_entry.description
  end
end

The most basic email delivery occurs with no parameters. Looking at our pending tests, if there is no :start_date or :end_date param, only time entries from today are sent out. That leads us to write the first two lines, creating a time entry from today and invoking the controller action.

time_entry = TimeEntry.create!(actual_start_time: Time.current)
post :send_invoice

We need to make sure that the time entry is included in the email, allowing us to finish the skeleton of our test.

...

last_email = ActionMailer::Base.deliveries.last
expect(last_email).to have_content time_entry.description

Running this code will throw errors, alerting us to what objects are necessary to get the test running.

After running the test for the first time, the code complains about validation errors on the time entry.

validation_error

Reading through the errors, let’s do the simplest thing to make one of the errors go away. We’ll add an actual_end_time to the time entry.

time_entry = TimeEntry.create!(actual_start_time: Time.current, actual_end_time: Time.current + 2.minutes)

We run the tests again. It now complains about a missing attribute.

bill_error

We jump into the model, and the only way to make that bill rate validation go away is to create a Rate object. We run it again and discover that the Rate object requires a Task object. Following the errors leaves us with the following setup code:

let(:client) { Client.create! }
let(:project) { Project.create!(client: client) }
let(:task) { Task.create!(:task, project: project) }
let(:user) { User.create!(email: 'user@example.com', password: 'password') }

Once the setup objects are put together, the test passes!

require 'rails_helper'

describe TimeEntriesController do
  let(:client) { Client.create! }
  let(:project) { Project.create!(client: client) }
  let(:task) { Task.create!(:task, project: project) }
  let(:user) { User.create!(email: 'user@example.com', password: 'password') }

  describe '#send_entries' do
    it 'delivers an email' do
      time_entry = TimeEntry.create!(actual_start_time: Time.current, actual_end_time: Time.current, task: task, user: user)
      post :send_invoice

      last_email = ActionMailer::Base.deliveries.last
      expect(last_email).to have_content time_entry.description
    end
  end

Whenever you get a test passing, you want to ensure that the test can also fail. The condition to make the test fail is to get the time entry’s #actual_start_time attribute outside of the range of the query. For the sake of simplicity, you’ll need to trust me that this test is legitimate.

We can get the other basic tests running, now that we have the setup code in place.

it 'delivers an email' do
  post :send_entries
  last_email = ActionMailer::Base.deliveries.last
  expect(last_email).to have_content time_entry.description
end

it 'flashes a success message' do
  post :send_entries
  expect(flash[:success]).to eq("Time Entries were successfully sent to you.")
end

it 'redirects to root path' do
  post :send_entries
  expect(response).to redirect_to(root_path)
end

We go through this cycle of setting up the skeleton of the test and letting the failures guide us to a green test for the rest of the pending tests. The test code ends up being 113 lines. I’ll post it here for completion’s sake, but understanding the process of getting the pending tests passing is the core idea of the bootstrapping section.

require 'rails_helper'

describe TimeEntriesController do
  let(:client) { Client.create! }
  let(:project) { Project.create!(client: client) }
  let(:task) { Task.create!(:task, project: project) }
  let(:user) { User.create!(email: 'user@example.com', password: 'password') }

  before do
    sign_in(user)
  end

  describe '#send_entries' do
    let(:yesterday) { Time.current - 1.day }
    let(:tomorrow) { Time.current + 1.day }
    let!(:time_entry) do
      TimeEntry.create!(actual_start_time: Time.current, actual_end_time: Time.current, task: task, user: user)
    end
    let!(:yesterday_time_entry) do
      TimeEntry.create!(actual_start_time: yesterday, actual_end_time: Time.current, task: task, user: user, description: 'Time Entry From Yesterday')
    end
    let!(:tomorrow_time_entry) do
      TimeEntry.create!(actual_start_time: tomorrow, actual_end_time: tomorrow + 1.day, task: task, user: user, description: 'Time Entry From Tomorrow')
    end

    it 'delivers an email' do
      post :send_entries
      last_email = ActionMailer::Base.deliveries.last
      expect(last_email).to have_content time_entry.description
    end

    it 'flashes a success message' do
      post :send_entries
      expect(flash[:success]).to eq("Time Entries were successfully sent to you.")
    end

    it 'redirects to root path' do
      post :send_entries
      expect(response).to redirect_to(root_path)
    end


    context 'start date is present' do
      let(:query_params) do
        { start_date: yesterday }
      end

      context 'end date is present' do
        before do
          query_params.merge!(end_date: tomorrow)
        end

        it 'sends time entries between start date and end date' do
          post :send_entries, query_params
          last_email = ActionMailer::Base.deliveries.last
          expect(last_email).to have_content time_entry.description
          expect(last_email).to have_content yesterday_time_entry.description
          expect(last_email).to have_content tomorrow_time_entry.description
        end
      end

      context 'end date is not present' do
        before do
          query_params.merge!(end_date: nil)
        end

        it 'sends entries between start date and today' do
          post :send_entries, query_params
          last_email = ActionMailer::Base.deliveries.last
          expect(last_email).to have_content time_entry.description
          expect(last_email).to have_content yesterday_time_entry.description
          # The tomorrow time entry is not in the query range
          expect(last_email).not_to have_content tomorrow_time_entry.description
        end
      end
    end

    context 'start date is not present' do
      let(:query_params) do
        { start_date: nil }
      end
      context 'end date is present' do
        before do
          query_params.merge!(end_date: tomorrow)
        end

        it 'sends entries between today and end date' do
          post :send_entries, query_params
          last_email = ActionMailer::Base.deliveries.last
          expect(last_email).to have_content time_entry.description
          # The yesterday time entry is not in the query range
          expect(last_email).not_to have_content yesterday_time_entry.description
          expect(last_email).to have_content tomorrow_time_entry.description
        end
      end

      context 'end date is not present' do
        before do
          query_params.merge!(end_date: nil)
        end

        it 'sends entries only from today' do
          post :send_entries, query_params
          last_email = ActionMailer::Base.deliveries.last
          expect(last_email).to have_content time_entry.description
          # Both the tomorrow and yesterday time entries are not in the query range
          expect(last_email).not_to have_content yesterday_time_entry.description
          expect(last_email).not_to have_content tomorrow_time_entry.description
        end
      end
    end
  end
end

We are now completely safe to refactor.

The Extraction Phase

When you have full coverage, refactor aggressively down to the models or extract plain Ruby objects.

We have complete coverage of our controller action. We can now extract the query logic into its own class.

Reading the controller code, there are three main concepts in the time entry query. We have a start_date, end_date, and time_entries. Those three concepts are going to guide the API of the TimeEntryQuery class.

Start with writing the code you’d love to have. You have a full battery of tests behind you, so you are completely safe.

class TimeEntriesController < ApplicationController

  def send_entries
    time_entries = TimeEntryQuery.new(start_date: params[:start_date], end_date: params[:end_date]).time_entries
    UserMailer.send_time_entries(current_user, time_entries).deliver_now
    flash[:success] = "Time Entries were successfully sent to you."
    redirect_to root_path
  end
end

The code we’d like to have will pass the parameters into a new TimeEntryQuery object and then calls the #time_entries method to perform the actual query.

controller_extraction

Our TimeEntryQuery class will look like the following, with all the controller tests still passing:

class TimeEntryQuery
  attr_reader :start_date, :end_date

  def initialize(start_date: nil, end_date: nil)
    @start_date = parse_date(start_date)
    @end_date   = parse_date(end_date)
  end

  def time_entries
    TimeEntry.order("actual_start_time DESC")
          .where("(actual_hours IS NOT NULL OR bill_amount IS NOT NULL) AND actual_start_time >= ? AND actual_start_time <= ?", start_date.beginning_of_day, end_date.end_of_day)
  end

  private

  def parse_date(date)
    if date
      Date.parse(date)
    else
      Time.zone.today
    end
  end
end

The magic sauce happens in the #initialize method. If either the :start_date or the :end_date don’t get specified or are nil, then the query defaults to today Time.zone.today.

The date parsing lives in a private #parse_date method, because the API of this TimeEntryQuery class should only return TimeEntry objects with the #time_entries method and do nothing else. You can set (or not set) :start_date and :end_date at your leisure and perform a query based off of them. That’s what our little class here does!

With our tests passing and our code refactored, let’s call the query from inside the mailer and fulfill our destiny!

fix_the_problem

With our refactored query, the mailer only sees a super small surface area into how the TimeEntryQuery works. We can reuse our query code anywhere in the application, our email gets delivered in the background, and our mailer method remains super lean!

We’ve got one more step to go.

The Test Refactor Phase

Refactor your controller integration tests into unit tests.

Our controller tests no longer need to account for all the edge cases, like having a :start_date param but not having an :end_date param. All of the edge cases are being handled by the TimeEntryQuery class.

Refactoring our controller tests down to the TimeEntryQuery class will help document our new class. As a bonus, we’ll get faster tests since unit tests are faster than controller integration tests.

The tests for our TimeEntryQuery class end up being aesthetically pleasing.

require 'rails_helper'

describe TimeEntryQuery do
  let(:client) { Client.create! }
  let(:project) { Project.create!(client: client) }
  let(:task) { Task.create!(:task, project: project) }
  let(:user) { User.create!(email: 'user@example.com', password: 'password') }

  describe '#time_entries' do
    let(:yesterday) { Time.current - 1.day }
    let(:tomorrow) { Time.current + 1.day }
    let!(:time_entry) do
      TimeEntry.create!(actual_start_time: Time.current, actual_end_time: Time.current, task: task, user: user)
    end
    let!(:yesterday_time_entry) do
      TimeEntry.create!(actual_start_time: yesterday, actual_end_time: Time.current, task: task, user: user, description: 'Time Entry From Yesterday')
    end
    let!(:tomorrow_time_entry) do
      TimeEntry.create!(actual_start_time: tomorrow, actual_end_time: tomorrow + 1.day, task: task, user: user, description: 'Time Entry From Tomorrow')
    end

    context 'start date and end date are supplied' do
      subject { described_class.new(start_date: yesterday.to_s, end_date: tomorrow.to_s).time_entries }
      it 'returns time_entries between start date and end date' do
        expect(subject).to include time_entry
        expect(subject).to include yesterday_time_entry
        expect(subject).to include tomorrow_time_entry
      end
    end

    context 'only start date is present' do
      subject { described_class.new(start_date: yesterday.to_s).time_entries }
      it 'returns time_entries between start date and today' do
        expect(subject).to include time_entry
        expect(subject).to include yesterday_time_entry
        expect(subject).not_to include tomorrow_time_entry
      end
    end

    context 'only end date is present' do
      subject { described_class.new(end_date: tomorrow.to_s).time_entries }
      it 'returns time_entries between today and end date' do
        expect(subject).to include time_entry
        expect(subject).not_to include yesterday_time_entry
        expect(subject).to include tomorrow_time_entry
      end
    end

    context 'no start date and no end date' do
      subject { described_class.new.time_entries }
      it 'returns time_entries from today' do
        expect(subject).to include time_entry
        expect(subject).not_to include yesterday_time_entry
        expect(subject).not_to include tomorrow_time_entry
      end
    end
  end
end

Our controller tests get leaned out and explicitly describe the goal of the controller action now, free of edge cases.

require 'rails_helper'

describe TimeEntriesController do
  let(:client) { Client.create! }
  let(:project) { Project.create!(client: client) }
  let(:task) { Task.create!(:task, project: project) }
  let(:user) { User.create!(email: 'user@example.com', password: 'password') }

  before do
    sign_in(user)
  end

  describe '#send_entries' do
    let(:yesterday) { Time.current - 1.day }
    let(:tomorrow) { Time.current + 1.day }
    let!(:time_entry) do
      TimeEntry.create!(actual_start_time: Time.current, actual_end_time: Time.current, task: task, user: user)
    end
    let!(:yesterday_time_entry) do
      TimeEntry.create!(actual_start_time: yesterday, actual_end_time: Time.current, task: task, user: user, description: 'Time Entry From Yesterday')
    end
    let!(:tomorrow_time_entry) do
      TimeEntry.create!(actual_start_time: tomorrow, actual_end_time: tomorrow + 1.day, task: task, user: user, description: 'Time Entry From Tomorrow')
    end

    it 'delivers an email' do
      post :send_entries
      last_email = ActionMailer::Base.deliveries.last
      expect(last_email).to have_content time_entry.description
    end

    it 'flashes a success message' do
      post :send_entries
      expect(flash[:success]).to eq("Time Entries were successfully sent to you.")
    end

    it 'redirects to root path' do
      post :send_entries
      expect(response).to redirect_to(root_path)
    end
  end
end

Wrapping Up

Rails controllers have multiple responsibilities and integrate all of the code in your application. This makes them a breeding ground for untested, bad code.

This article prescribes a blueprint for wrangling legacy controller code:

  1. Wrap controller code with pending tests to gain understanding.
  2. Write out the tests. Let test failures guide you to green tests.
  3. When you have full coverage, refactor aggressively down to the models or extract plain Ruby objects.
  4. Refactor your controller integration tests into unit tests.

Armed with these steps, you can wrap your mind around unknown codebases, get them under test, and refactor safely.

You can read the commits of this article’s code refactor on GitHub.

Subscribe via Email

Over 60,000 people from companies like Netflix, Apple, Spotify and O'Reilly are reading our articles.
Subscribe to receive a weekly newsletter with articles around Continuous Integration, Docker, and software development best practices.



We promise that we won't spam you. You can unsubscribe any time.

Join the Discussion

Leave us some comments on what you think about this topic or if you like to add something.

  • I agree with your approach here, and I think it’s a great illustration of how proper testing makes code easier to change. One might see all of those controller tests and think “oh, all of those tests focused on the controller, I don’t want to test to that level because what-if-things-change, I don’t want to rewrite them.” But as you show, it’s very simple to use the tests to verify the behavior during the refactor, and then move the tests over to it. This is exactly the approach that I use in a lot of code refactors, and it works out well. Bravo!

    I like your analogy as well… the controllers being the bouncers of the application. I liked what Uncle Bob said about Rails apps, about how they should be the “delivery mechanism” of your application. Your application contains lots of logic that should work independently of the scheme used to activate that logic. We often think of Rails apps and think it must be Rails, but look at how quickly Rails apps are moving to the background processing. That’s essentially two applications, but that share code. Our controllers should handle the details that only exist due to the fact that the application is being called as a web application, and then leave the rest into separate classes.

    The only disagreement I have with your post is how you talk of unit tests versus controller tests or integration tests. But it’s not really a gripe with your blog post as it is with the Rails community. With Rails, we’ve come to think of “unit tests” as something that targets models and the like, and controller tests as a separate matter that requires the test to run through the web process. That’s not how the terms are used outside of the Rails community.

    Outside, “unit test” means a test against some sort of unit. That unit is usually mean to be a small one, like a single class. Integration tests involve multiple units, usually to see how they can work when they’re used together. With those definitions… ALL of your tests in this blog post are integration tests, as they hit the database… and go through the AR code and validations…. and which has to run with the Rails environment set up and running.

    It’s not true that controller’s can’t be unit tested. They can, and do… in fact, practically all of my controller tests are unit tests. I mock out the controller’s dependencies, leaving my controller tests focused precisely on the orchestration of the web request & response — and nothing else. And my tests run very quickly! Then I have additional unit tests to cover the code that the controller calls. My only integration tests (which aren’t many) will make a request to verify that all of these units work together, but nothing more. Fast tests, high coverage, and fewer dependencies.

    Great post!!! :)

    • Joshua Plicque

      Thank you for this feedback, Darren! I love and appreciate your perspective. I figured my blanket statement, “controllers can’t be unit tested”, would be up for debate. You’re making a valid argument when you say that you can mock everything out and bam, it’s a unit test!

      My rebuttal is that you’re still running the test through the router (integration). In addition, even if you prevent the controller tests from rendering the views in your unit tests, that’s not how they will operate in production. A controller unit test is not fully effective because the view rendering side effect will occur in production. If you unit test the controller (without rendering the view), then you aren’t fully testing what the controller action does. The view could be completely broken and your controller test would pass, but the code would break in production.

      Thank you for your positive and constructive feedback. Your thoughts are always well considered and communicated clearly. Cheers!

      • I’ve included a link below of an example Rails controller test. I wrote it free-hand to show how a controller test could be written in Rails, but the code hasn’t been run. But it’ll give you a general idea of how it could work.

        You said a couple things that I’d like to respond to, only to clear up what I mean to unit test – not to drag you into a long debate or anything. :)

        > My rebuttal is that you’re still running the test through the router (integration).

        But if you were unit testing the controller, you wouldn’t run it through the router. A Rails controller is a class, and an action is a method on the class. There’s nothing that stops me from new’ing up a controller and calling the action on it.

        > The view could be completely broken and your controller test would pass, but the code would break in production.

        That is true, but that’s the very nature of a unit test. We test the “unit” in isolation from all of the other units. Any of those other units could fail, but given the scenarios that we establish for our unit, we can be confident that our unit under test will work.

        So you are absolutely right in that I could unit test my controller and miss a possible error in the view. I mitigate that with the removal of all logic from the views (which makes them practically impossible to fail), selective “smoke” integration tests (with Spinach or Cucumber), a testing environment for QA, intense error tracking on the site, and website analytics to report how the site is functioning to the business. Sounds like a lot, but it’s not… and it saves a lot of time that’d normally be spent fighting integration tests.

        In my experience, the Rails community has a very strange attitude towards testing. We test our Rails apps, and we test them with integration tests because we think we’re being very thorough. But since they’re integration tests, we feel the natural pain of integration tests very quickly, and we tend to stop testing the finer details. So, the span of each test in the application is very high, but the true test coverage is very low and the tests are very brittle. So… they fail a few months into a web application, and we shrug and say “oh testing!” but it’s really our fault.

        https://gist.github.com/darrencauthon/0c11c7fd24f82733bd12

        • One quick note about my example…. you’ll find that I’m using Active Record, which technically makes my “unit” test an integration test.

          I do this because Active Record is an absolute beast, and mocking out its behaviors would be incredibly difficult. So I don’t… but I keep my database design very simple, I keep validations separate from my model (which is **ESSENTIAL** to simple Rails testing), and I forgo the relationship management (which is the most expensive part of any ORM).

          The funny thing about this is that most people instantly point to the database when talking about slow tests, but databases are really, really fast. My tests still run in milliseconds, even with the database calls — and mocking them out would not save much time. The real reason Rails tests tend to be so slow is that they’re running through the entire web request/response cycle, and Rails testing requires a lot of complicated setups due the way we use/abuse Active Record.

        • Joshua Plicque

          Fair enough! Controllers CAN be unit tested!

          I’m of the opinion that they shouldn’t be, since it’s role is to integrate. I updated the post to reflect that.

  • Ildomar Grings

    Good job

  • MadBomber

    I also echo the Good Job comment.

  • Pingback: 週刊Railsウォッチ(20161218)Ruby 2.4ではFloat#roundの動作が変わる、デフォルトのプライマリキーをBIGINTに変更ほか()