Smell A Smell Episode 1: Long Method

Written by Kyle Chilcutt

At UserTesting, refactoring code is a way of life. In this first installment of "Smell a Smell," learn to refactor long methods.

One easy smell to identify in your code is “Long Method.” Methods that are longer than 10 lines are generally viewed as potential problem areas and can harm the readability and maintainability of your code. In this post, I’m going to demonstrate cleaning up this code smell with some simple refactoring!

Here’s a segment of code guilty of “Long Method.”

def setup!
  # general attributes for all orders
  invoice_attrs = {
    user: self.order.user,
    account: self.order.account,
    user_company_name: order.user.company_name,
    user_name: order.user.display_name,
    user_phone: order.user.phone_number,
  }

  # refunds should already set these two attributes
  unless self.transaction_type == REFUND_TRANSACTION_TYPE
    invoice_attrs.merge!(
      dollar_total: self.order.dollar_total,
      credit_total: self.order.credit_total
    )
  end

  # specialized attributes for the different order types
  study = order.studies.first
  if study and study_order? # standard user test order
    invoice_attrs.merge!(
      number_of_users: study.number_of_users,
      user_email: study.notification_email
    )
  elsif credit_order? # credit order
    invoice_attrs.merge!(
      number_of_credits: order.credits.map(&:original_value).sum,
    )
  end

  update_attributes!(invoice_attrs)
  self
end

We want to improve the readability of this code by refactoring it. As I mentioned above, we’re going to try to decompose this method into smaller, more readable methods by using the “Extract Method” refactor.

Before we start, it’s a good idea to identify the behavior of our method so that we can ensure that the code behaves the same after our refactor. It can often be tempting to add new functionality or to change how the code works but this can cause errors and should be resisted if possible. Let’s take a closer look at what our method is doing.

This #setup! method is building up a hash of changes to apply to our model and then updates all of them at once using mass-assignment. The attributes and values to update are stored in the variable invoice_attrs.

It’s also worth noting the arguments and return value of the method. So that I can contain this refactor to only this class, I want to make sure that all of the blackbox interfaces remain the same. In this case, it means that this method should continue to be called with no arguments and return the saved instance of the model.

With all this in mind, let’s get started! The first step is to identify segments of the code I want to extract and to create new methods from them. Luckily for me in this situation, our sections seem pretty well delimited by comments! A comment that explains a section of code is often an indicator of a block of code that we can extract into its own method. Let’s take this first section as an example.

  # general attributes for all orders
  invoice_attrs = {
    user: self.order.user,
    account: self.order.account,
    user_company_name: order.user.company_name,
    user_name: order.user.display_name,
    user_phone: order.user.phone_number,
  }

This section does two things: creates the invoice_attrs variable and sets general attributes for our invoice. We can extract this into a method that does the same thing. I’ll also take this opportunity to remove redundant calls to self.

def set_general_attributes(attrs)
  attrs.merge!(user: order.user,
               account: order.account,
               user_company_name: user.company_name,
               user_name: user.display_name,
               user_phone: user.phone_number)
end

We can now call this method directly from our setup! method:

def setup!
  attrs = {}
  set_general_attributes(attrs)

  # ...
end

That’s the basic gist of this! I’m going to apply the same “Extract Method” pattern a few more times and we’ll fast forward to the end result.

def setup!
  attrs = {}
  set_general_attributes(attrs)
  set_transaction_attributes(attrs)
  set_order_attributes(attrs)
  update_attributes(attrs)
  self
end

private

def set_general_attributes(attrs)
  attrs.merge!(user: order.user,
               account: order.account,
               user_company_name: user.company_name,
               user_name: user.display_name,
               user_phone: user.phone_number)
end

def set_transaction_attributes(attrs)
  return if self.transaction_type == REFUND_TRANSACTION_TYPE
  attrs.merge!(dollar_total: order.dollar_total,
               credit_total: order.credit_total)
end

def set_order_attributes(attrs)
  if study && study_order?
    attrs.merge!(number_of_users: study.number_of_users,
                 user_email: study.notification_email)
  elsif credit_order?
    attrs.merge!(number_of_credits: order.credits.map(&:original_value).sum)
  end
end

That’s a lot better! Now we can clearly see that #setup! is a procedural method that will assign a bunch of attributes and return self.

There’s just one small tweak I’d like to do to finish this off. One of the key aspects of this method is that it updates all of the attributes in a single transaction, but there’s a simpler way to accomplish the same thing without creating a temporary hash. As this is an ActiveRecord model, we can simply assign the changes directly to the model instance and then #save! the instance which will have the same result!

def setup!
  assign_general_attributes
  assign_transaction_attributes
  assign_order_attributes
  save!
  self
end

private

def assign_general_attributes
  assign_attributes(user: order.user,
                    account: order.account,
                    user_company_name: user.company_name,
                    user_name: user.display_name,
                    user_phone: user.phone_number)
end

def assign_transaction_attributes
  return if self.transaction_type == REFUND_TRANSACTION_TYPE
  assign_attributes(dollar_total: order.dollar_total,
                    credit_total: order.credit_total)
end

def assign_order_attributes
  if study && study_order?
    assign_attributes(number_of_users: study.number_of_users,
                      user_email: study.notification_email)
  elsif credit_order?
    assign_attributes(number_of_credits: order.credits.map(&:original_value).sum)
  end
end

Refactoring can really help with code maintainability and should be practiced regularly! To learn more about refactoring, we recommend picking up a copy of the Refactoring book. Stay tuned for the next episode and more tips!