By 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:
Appointmenthas many different responsibilities (a violation of theSingle Responsibility Principle), one of these responsibilities being*recording transactions. By adding more transaction-related code to theAppointmentclass, **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
Appointmentclass 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.
Molly Struve, Sr. Site Reliability Engineer, Netflix
