Refactoring Ruby using Sprout Classes

One of the hairiest challenges of working with some legacy applications is that the code wasn't written to be testable. So writing meaningful tests is difficult or impossible.

It's a chicken-and-egg problem: in order to write tests for the legacy application, you have to change the code, but you can't confidently change the code without first writing tests for it!

How do you deal with this paradox?

This is one of the many subjects covered in Michael Feathers' excellent Working Effectively with Legacy Code. Today I'm going to zoom in on one particular technique from the book called Sprout Class.

Get ready for some LEGACY CODE!

Let's take a look at this old ActiveRecord class called Appointment. It's pretty long, and in real life it's 100's of lines longer.

class Appointment < ActiveRecord::Base 
  has_many :appointment_services, :dependent => :destroy
  has_many :services, :through => :appointment_services
  has_many :appointment_products, :dependent => :destroy
  has_many :products, :through => :appointment_products
  has_many :payments, :dependent => :destroy
  has_many :transaction_items
  belongs_to :client
  belongs_to :stylist
  belongs_to :time_block_type

  def record_transactions
    transaction_items.destroy_all
    if paid_for?
      save_service_transaction_items
      save_product_transaction_items
      save_tip_transaction_item
    end
  end

  def save_service_transaction_items
    appointment_services.reload.each { |s| s.save_transaction_item(self.id) }
  end

  def save_product_transaction_items
    appointment_products.reload.each { |p| p.save_transaction_item(self.id) }
  end

  def save_tip_transaction_item
    TransactionItem.create!(
      :appointment_id => self.id,
      :stylist_id => self.stylist_id,
      :label => "Tip",
      :price => self.tip,
      :transaction_item_type_id => TransactionItemType.find_or_create_by_code("TIP").id
    )
  end
end

Adding some features

If we're asked to add some new functionality to the transaction-reporting area, but the Appointment class has too many dependencies to be testable without a lot of refactoring, how do we proceed?

One option is to just make the change:

def record_transactions
  transaction_items.destroy_all
  if paid_for?
    save_service_transaction_items
    save_product_transaction_items
    save_tip_transaction_item
    send_thank_you_email_to_client # New code
  end
end

def send_thank_you_email_to_client
  ThankYouMailer.thank_you_email(self).deliver
end

That kind of sucks

There are two problems with the above code:

Appointment has many different responsibilities (a violation of theSingle Responsibility Principle), one of these responsibilities being*recording transactions. By adding more transaction-related code to the Appointment class, **we're making the code a little bit worse*.
we could maybe write a new integration test and check to see that the email made it across, but since we wouldn't have the Appointment class in a testable state, we couldn't add any unit tests. We'd be adding more untested code, which is of course bad. (Michael Feathers, in fact, defines legacy code as "code without tests," so we'd be adding_more_ legacy code to the legacy code.)

Chunking it up is better

A better solution than simply adding the new code inline would be to extract the transaction-recording behavior into its own class. We'll call it, say,TransactionRecorder:

class TransactionRecorder 
  def initialize(options)
    @appointment_id       = options[:appointment_id]
    @appointment_services = options[:appointment_services]
    @appointment_products = options[:appointment_products]
    @stylist_id           = options[:stylist_id]
    @tip                  = options[:tip]
  end

  def run
    save_service_transaction_items(@appointment_services)
    save_product_transaction_items(@appointment_products)
    save_tip_transaction_item(@appointment_id, @stylist_id, @tip_amount)
  end

  def save_service_transaction_items(appointment_services)
    appointment_services.each { |s| s.save_transaction_item(appointment_id) }
  end

  def save_product_transaction_items(appointment_products)
    appointment_products.each { |p| p.save_transaction_item(appointment_id) }
  end

  def save_tip_transaction_item(appointment_id, stylist_id, tip)
    TransactionItem.create!(
      appointment_id: appointment_id,
      stylist_id: stylist_id,
      label: "Tip",
      price: tip,
      transaction_item_type_id: TransactionItemType.find_or_create_by_code("TIP").id
    )  
  end
end

The payoff

Then Appointment can get pared down to just this:

class Appointment < ActiveRecord::Base 
  has_many :appointment_services, :dependent => :destroy
  has_many :services, :through => :appointment_services
  has_many :appointment_products, :dependent => :destroy
  has_many :products, :through => :appointment_products
  has_many :payments, :dependent => :destroy
  has_many :transaction_items
  belongs_to :client
  belongs_to :stylist
  belongs_to :time_block_type

  def record_transactions
    transaction_items.destroy_all
    if paid_for?
      TransactionRecorder.new(
        appointment_id: id,
        appointment_services: appointment_services,
        appointment_products: appointment_products,
        stylist_id: stylist_id,
        tip: tip
      ).run
    end
  end
end

We're still modifying the code in Appointment, which can't be tested, but now we_can_ test everything in TransactionRecorder, and since we've changed each function to accept arguments rather than use instance variables, we can even test each function in isolation. So we're in a much better spot now than when we started.