Smell A Smell Episode 3: Couplers

Written by Jerrod Blavos

At UserTesting, refactoring code is a way of life. In this third installment of "Smell a Smell," learn to identify and refactor couplers.

In the spirit of investigating the “coupling” code smells, we will be looking at smells that are often found together — specifically Feature Envy and Message Chains. We will also make references to the other smells, Inappropriate Intimacy and Middle Man. We will look at an example and work through refactoring it one step at a time. Imagine we have integration tests running in the background, because we always refactor using tests.

To identify feature envy, Look for methods that are accessing data belonging to another object more frequently than it’s own. Here is a small example - notice how the only data accessed is that belonging to the yard?

class House
  def do_yard_work
    if yard.bushes.unruly?
      yard.bushes.trim
    elsif yard.grass.too_tall?
      yard.grass.trim
    end
  end
end  

Message chains are one of the more common smells in Rails applications. Long sequences of messages linked together are a sure sign you need to look at your dependencies. In Ruby it can look like:

cars.last.rear_bumper.bumper_stickers.first.title

Step one: Identify the smells and break them down

We will start by looking at some simplified real world code from the UserTesting codebase. Assume that we have orders, which occasionally need to be refunded. To generate the refund, we need to know how much the refund is worth.

class Order
  has_many :refund_payments

  def refund(refund_payment_attributes)
    refund = refund_payments.new(refund_payment_attributes)
    user.credits.create(
      price_per_test:  refund.new_price_per_test,
    )
  end
end

class RefundPayment
  belongs_to: :order

  def new_price_per_test
    if order and order.prepaid_credit_payments.blank? and order.studies.present?
      (order.dollar_total.to_f / order.studies.map(&:number_of_users).sum.to_f).to_f
    elsif order && order.prepaid_credit_payments.last
      order.prepaid_credit_payments.last.credit.price_per_test.to_f
    else
      0.00
    end
  end
end

Step two: Move that Method!

Now that we know what to look for, it should be obvious that the new_price_per_test method is suffering from Feature Envy, a minor case of Inappropriate Intimacy, and just a touch of being a Middle Man. Notice how the method is solely concerned with data belonging to the order - self is not referenced once in this method.

The recommended refactor when encountering feature envy is Move Method, and as a byproduct of moving the method we have also removed the middle man. It is obvious that the target of our move is the order class, as it is the object receiving every message in the method.

class Order
  has_many :refund_payments

  def refund(refund_payment_attributes)
    refund = refund_payments.new(refund_payment_attributes)
    user.credits.create(
      price_per_test:  new_price_per_test,
    )
    refund
  end

  def new_price_per_test
    if prepaid_credit_payments.blank? and studies.present?
      (dollar_total.to_f / studies.map(&:number_of_users).sum.to_f).to_f
    elsif prepaid_credit_payments.last
      prepaid_credit_payments.last.credit.price_per_test.to_f
    else
      0.00
    end
  end
end

Step three: Break the chain into explaining variables

We now are no longer concerned with the RefundPayment class to determine the price, and the new_price_per_test method is greatly simplified by removing the need to refer back to it’s order.

This concludes our feature envy refactor, but we will continue on to address the message chain that remains in the new_price_per_test method. To do this, we will break the chain into smaller explaining variables and use these to determine our next steps.

class Order
  has_many :refund_payments

  def refund(refund_payment_attributes)
    refund = refund_payments.new(refund_payment_attributes)
    user.credits.create(
      price_per_test:  new_price_per_test,
    )
    refund
  end

  def new_price_per_test
    if prepaid_credit_payments.blank? and studies.present?
      (dollar_total.to_f / studies.map(&:number_of_users).sum.to_f).to_f
    elsif prepaid_credit_payments.last
      last_payment = prepaid_credit_payments.last
      payment_credit = last_payment.credit
      payment_credit.price_per_test.to_f
    else
      0.00
    end
  end
end

Step four: extract methods for each explaining variable

This is better, as we have exposed the dependencies we need to address. We extract methods for each variable, one at a time, while making sure our tests remain green.

class Order
  has_many :refund_payments

  def refund(refund_payment_attributes)
    refund = refund_payments.new(refund_payment_attributes)
    user.credits.create(
      price_per_test:  new_price_per_test,
    )
    refund
  end

  def new_price_per_test
    if prepaid_credit_payments.blank? and studies.present?
      (dollar_total.to_f / studies.map(&:number_of_users).sum.to_f).to_f
    elsif prepaid_credit_payments.last
      # last_payment = prepaid_credit_payments.last
      payment_credit = last_payment.credit
      payment_credit.price_per_test.to_f
    else
      0.00
    end
  end

  def last_payment
    prepaid_credit_payments.last
  end
end

Step five: Rinse. Repeat.

With this method extracted, we will continue the same process of extracting our explaining variable into a method while making sure our tests are still passing.

class Order
  has_many :refund_payments

  def refund(refund_payment_attributes)
    refund = refund_payments.new(refund_payment_attributes)
    user.credits.create(
      price_per_test:  new_price_per_test,
    )
    refund
  end

  def new_price_per_test
    if prepaid_credit_payments.blank? and studies.present?
      (dollar_total.to_f / studies.map(&:number_of_users).sum.to_f).to_f
    elsif prepaid_credit_payments.last
      # last_payment = prepaid_credit_payments.last
      # payment_credit = last_payment.credit
      payment_credit.price_per_test.to_f
    else
      0.00
    end
  end

  def last_payment
    prepaid_credit_payments.last
  end

  def payment_credit
    last_payment.credit
  end
end

Step six: Delete unused code

We have now extracted methods for each of the links in our message chain, applying the middle man refactor along the way. The final step to our refactor is to remove the now-unused code.

class Order
  has_many :refund_payments

  def refund(refund_payment_attributes)
    refund = refund_payments.new(refund_payment_attributes)
    user.credits.create(
      price_per_test:  new_price_per_test,
    )
    refund
  end

  def new_price_per_test
    if prepaid_credit_payments.blank? and studies.present?
      (dollar_total.to_f / studies.map(&:number_of_users).sum.to_f).to_f
    elsif prepaid_credit_payments.last
      payment_credit.price_per_test.to_f
    else
      0.00
    end
  end

  def last_payment
    prepaid_credit_payments.last
  end

  def payment_credit
    last_payment.credit
  end
end

Though we still have dependencies within the class on prepaid_credit_payments and their credits, we have eliminated the need for RefundPayment in calculating our new_price_per_test.

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!