Refactoring for Readability

Photo by Fabian Grohs on Unsplash

At Beam, we make an effort to campsite our code as we add features and fix bugs. Over time, engineers develop a sense of how readable code is, and use this intuition (along with a formalized list of “code smells” that I talk about at the end of the article) to refactor troubling parts of a codebase. Code that has long methods, lots of temp variables, parameter lists that get passed to multiple functions, and methods that know more than they need to about the data model, tend to look ugly. Rewriting these methods with only readability in mind dovetails with the prescribed fixes for various smells pretty well.

Let’s take a small example from the Beam codebase, the CompanyInvoicesController, and show how some simple readability improvements can make life easier for the next engineer to come along. Some background:

• Beam sells insurance plans to companies.
• Companies have many insured members, some of whom may not currently have active insurance.
• We create a monthly invoice that totals up the cost of all employees plans.
• We present a list of these, plus an estimate of the next invoice, in a Rails index view.

The “Before” code:

class CompanyInvoicesController < ApplicationController
  before_action :set_company, only: [:index]
def index
    authorize @company, :show?
@invoices = @company.invoices.where(full_invoice: true).to_a
# add a projection for this month at the beginning
    @invoices.unshift(get_next_month_projection)
@invoices = @invoices.map { |invoice| CompanyInvoiceSerializer.new(invoice) }
respond_with @invoices
  end
private
# Use callbacks to share common setup or constraints between actions.
  def set_company
    @company = Company.find(params[:id])
  end
  def get_next_month_projection
    invoice_date = (1.month.from_now).beginning_of_month
    invoice = CompanyInvoice.new
    invoice.company = @company
    invoice.invoiced_at = invoice_date
    invoice.employees = @company.certs
      .only_unterminated
      .where("certs.effective_at <= ?", invoice_date).size
    invoice.total = @company.premium_amount.to_f
    invoice
  end
end

For code written in a hurry by a startup driven to quickly bring a product to market, this ain’t half-bad. That being said, there are some things here that jump out at me as being problems. The first problem I see is with the set_company before action:

before_action :set_company, only: [:index]
def index
   ...
end
private
def set_company
  @company = Company.find(params[:id])
end

#index is the only public method, so the only: [:index] is either premature optimization, or it was once needed but now is not. All the setter method does is set an instance variable, so that would be a good candidate to replace with a maybe monad:

def company
  @company ||= Company.find(params[:id])
end

Then, we can drop the @ off of the remaining references to company. The next thing that looks awkward is everything #index is doing with @invoices:

@invoices = company.invoices.where(full_invoice: true).to_a
@invoices.unshift(get_next_month_projection)
@invoices = @invoices.map { |invoice| CompanyInvoiceSerializer.new(invoice) }
respond_with @invoices

We start by taking an ActiveRecord query and casting it to an array so that we can call unshift on it. Then we are redefining @invoices with the output from a serializer, and then explicitly responding with the redefined variable.

query_results.to_a.unshift(new_thing) is syntactically the same as [new_thing] + query_results. It looks a lot nicer, and casting to an array happens under the hood. I want a method that abstracts all this away:

def invoices
  [next_month_projection] + company.invoices.full_invoices
end

full_invoices was an available scope on CompanyInvoice, so we were able to clear off some punctuation for free. Next, rather than redefine an instance variable, we can directly respond with invoices mapped through the serializer:

def index
  authorize company, :show?
 
  respond_with invoices.map { |invoice| CompanyInvoiceSerializer.new(invoice) }
end

Much nicer looking, and it does set the instance variable, in case the view depends on it (ours did not). The next thing to tackle is the get_next_month_projection private method.

def get_next_month_projection
  invoice_date = (1.month.from_now).beginning_of_month
  invoice = CompanyInvoice.new
  invoice.company = @company
  invoice.invoiced_at = invoice_date
  invoice.employees = @company.certs
    .only_unterminated
    .where(“certs.effective_at <= ?”, invoice_date).size
  invoice.total = @company.premium_amount.to_f
  invoice
end

The name is a minor annoyance; we can drop the get_ from the beginning and still understand the method’s purpose. A blank invoice object is instantiated, and then fields are set on it one statement at a time. Lastly, the invoice object is explicitly returned. I’ve seen patterns like this in the codebases of a few different companies. I don’t think of every temporary variable as a code smell, but a variable return as the last statement in a method really stands out to me as something that can be done more cleanly.

This method wants to be a simple ActiveRecord new statement, but we have to get all our variables in order first. The invoice_date temp variable is set to the date of the next invoice, so it can be replaced with a better-named method and an instance variable, as we did earlier with @company. We’re setting invoice.employees to a count of active employees, which would make a nice method of its own. Our two helper methods look like this:

def active_employee_count
  company
    .certs
    .only_unterminated
    .where(“certs.effective_at <= ?”, next_invoice_date)
    .size
end
 
def next_invoice_date
  @next_invoice_date ||= 1.month.from_now.beginning_of_month
end

Then, next_month_projection can be made a little more elegant:

def next_month_projection
  CompanyInvoice.new ({
    company: company,
    invoiced_at: next_invoice_date,
    employees: active_employee_count,
    total: company.premium_amount.to_f
  })
end

With only a small amount of effort, our complete class is more readable:

class CompanyInvoicesController < ApplicationController
  def index
    authorize company, :show?
    respond_with invoices.map { |invoice| CompanyInvoiceSerializer.new(invoice) }
  end
  private
  def company
    @company ||= Company.find(params[:id])
  end
  def next_month_projection
    CompanyInvoice.new ({
      company: company,
      invoiced_at: next_invoice_date,
      employees: active_employee_count,
      total: company.premium_amount.to_f
    })
  end
  def active_employee_count
    company
      .certs
      .only_unterminated
      .where("certs.effective_at <= ?", next_invoice_date)
      .size
  end
  def next_invoice_date
    @next_invoice_date ||= 1.month.from_now.beginning_of_month
  end
  def invoices
    [next_month_projection] + company.invoices.full_invoices
  end
end

With the code easier to read, the next engineer to come across it will have an easier time parsing what’s going on and will be able to more confidently make changes. For our 20 minute investment, we’ve saved hours of future engineering time.

Additionally, now that the code is more readable, some business and data flow problems start to stand out. For example, what’s the next invoice date if today is the first of the month? Now that it’s more apparent nothing ever saves it, should the next_month_projection invoice be committed to the database?

That would be enough for one pass, and at a startup with fires to put out, that might be all you have time for up front. Later the above process problems can be answered, the active employee count might be teased apart so it can be used with a helper scope in the Cert class, and maybe the next invoice date could be moved to the invoice model or a date helper class. Each of those changes introduces more risks, so before they’re attempted, we should double check that our unit tests cover all this behavior.

…which would make a good topic for our next scaling post!

Postscript: For more on this kind of refactoring, the following are recommended resources. Sandi Metz gave the Get a Whiff of This talk at RailsConf in 2016, which is a great introduction to the concept of “code smells”. There are a standard lexicon of smells that can be addressed with well-known recipes culled from Martin Fowler’s book Refactoring and Joshua Kerievsky’s Refactoring to Patterns. Fowler also makes freely available code samples of each recipe step.