Note: after I wrote this post I came up with a better example, which can be found here.

There are a number of reasons why it can often be difficult to add test coverage to legacy projects.

One barrier is the entanglement of dependencies that stands in the way of instantiating the objects you want to test.

Another barrier is the “wall of code” characteristic often present in legacy code: long, unstructured classes and methods with obfuscated variable names and mysterious behavior. Testing big things is a lot harder than testing small things.

In this example I’m going to demonstrate how to take a long, unstructured method, put tests on it, and then improve its design.

Legacy code example

Here’s a piece of crappy simulated legacy code:

require 'date' class CreditCard def initialize(number, expiration_date, brand) @number = number @expiration_date = expiration_date @brand = brand end def valid? month, year = @expiration_date.split('/').map(&:to_i) year += 2000 if DateTime.now.to_date < Date.new(year, month) return false else if @brand == 'American Express' @number.gsub(/\s+/, '').length == 15 else @number.gsub(/\s+/, '').length == 16 end end end end

Why is this code crappy? For one, the `valid?` method is too long to easily be understood at a glance. Secondly, it has a bug. If you give the class an expired card it will still tell you the card is valid.

> cc = CreditCard.new('3843111122223333', '02/17', 'Visa') => #<CreditCard:0x007fb8d9825890 @number="3843111122223333", @expiration_date="02/17", @brand="Visa"> > cc.valid? => true

If we want to fix this code there are a couple different approaches we could take.

Two Possible Approaches

Try to debug the code by tediously running it over and over on the console Write some tests

The second approach has the benefit of being not only less annoying but also permanent. If we write a test to enforce that the expiration date bug is not present, we can reasonably expect that our test will protect against this bug forever. If we just debug the code in the console, we get no such guarantee.

But it’s not always easy to just go ahead and start writing tests.

Obstacles to Writing Tests

There’s a certain obstacle to testing that’s often present in legacy code. That is, when you have a line or file that’s a bazillion lines long, it’s hard to isolate the piece of code you’re interested in testing. There are often dependencies tangled up in the code. You’re afraid to try to untangle the dependencies because you don’t want to change code you don’t understand. But in order to understand the code, you need to untangle the dependencies. It’s a tough catch-22 type situation.

The Solution

The solution I like to apply to this problem is Michael Feathers’ sprout method technique which he describes in Working Effectively with Legacy Code.

What we’re about to do is:

Write a test around the buggy area—expiration date validation—and watch it fail Extract the expiration date code into its own method so we can isolate the incorrect behavior Fix the bug and watch our test pass Let’s start by writing the first test. Writing the First Test I’ll write a test that says, “When the credit card’s expiration date is in the past, expect that card not to be valid.” require 'rspec' require './credit_card' describe CreditCard do describe '#valid?' do context 'expired' do it 'is not valid' do cc = CreditCard.new('3843111122223333', '02/17', 'Visa') expect(cc).not_to be_valid end end end end When I run this test it fails, as expected. Next I’ll break up this code a little bit by extracting a chunk of it into its own method. Extracting Our First Method The highlighted chunk below looks like a pretty good candidate to be separated. def valid? month, year = @expiration_date.split('/').map(&:to_i) year += 2000 if DateTime.now.to_date < Date.new(year, month) return false else if @brand == 'American Express' @number.gsub(/\s+/, '').length == 15 else @number.gsub(/\s+/, '').length == 16 end end end I’m going to extract it into a `number_is_right_length?` method. require 'date' class CreditCard def initialize(number, expiration_date, brand) @number = number @expiration_date = expiration_date @brand = brand end def valid? month, year = @expiration_date.split('/').map(&:to_i) year += 2000 if DateTime.now.to_date < Date.new(year, month) return false else number_is_right_length? end end def number_is_right_length? if @brand == 'American Express' @number.gsub(/\s+/, '').length == 15 else @number.gsub(/\s+/, '').length == 16 end end end Now that I have this functionality in its own method, I’ll write a test for just that part of the code, in isolation. require 'rspec' require './credit_card' describe CreditCard do describe '#valid?' do context 'expired' do it 'is not valid' do cc = CreditCard.new('3843111122223333', '02/17', 'Visa') expect(cc).not_to be_valid end end end describe '#number_is_right_length?' do context 'no spaces' do context 'right length' do it 'returns true' do cc = CreditCard.new('3843111122223333', '02/17', 'Visa') expect(cc.number_is_right_length?).to be true end end end end end If I run this test it will pass. There’s something I don’t like about this, though. The expiration date of the card doesn’t have anything to do with the validity of the length of the card number. The extra setup data is distracting. A future maintainer might look at the test and assume that the expiration date is necessary. Removing Extraneous Setup Data It would be nice if we could test just the number length by itself. It would be nice if our test could look like this: require 'rspec' require './credit_card' describe CreditCard do describe '#valid?' do context 'expired' do it 'is not valid' do cc = CreditCard.new('3843111122223333', '02/17', 'Visa') expect(cc).not_to be_valid end end end describe '#number_is_right_length?' do context 'no spaces' do context 'right length' do it 'returns true' do valid = CreditCard.number_is_right_length?('Visa', '3843111122223333') expect(valid).to be true end end end end end In order to make our new `number_is_right_length?` method more testable in isolation, I’m going to change it from an instance method to a class method. Instead of using the `@brand` and `@number` instance variables, I’m going to use dependency injection so the `number_is_right_length?` doesn’t rely on a `CreditCard` instance having been created first. require 'date' class CreditCard def initialize(number, expiration_date, brand) @number = number @expiration_date = expiration_date @brand = brand end def valid? month, year = @expiration_date.split('/').map(&:to_i) year += 2000 if DateTime.now.to_date < Date.new(year, month) return false else self.class.number_is_right_length?(@brand, @number) end end def self.number_is_right_length?(brand, number) if brand == 'American Express' number.gsub(/\s+/, '').length == 15 else number.gsub(/\s+/, '').length == 16 end end end Extracting Expiration-Related Code Now I’m going to extract the expiration-related code into its own method. That code is highlighted below. def valid? month, year = @expiration_date.split('/').map(&:to_i) year += 2000 if DateTime.now.to_date < Date.new(year, month) return false else self.class.number_is_right_length?(@brand, @number) end end That chunk of code will go out of the `valid?` method and into a new `expired?` method. require 'date' class CreditCard def initialize(number, expiration_date, brand) @number = number @expiration_date = expiration_date @brand = brand end def valid? if expired? false else self.class.number_is_right_length?(@brand, @number) end end def expired? month, year = @expiration_date.split('/').map(&:to_i) year += 2000 DateTime.now.to_date < Date.new(year, month) end def self.number_is_right_length?(brand, number) if brand == 'American Express' number.gsub(/\s+/, '').length == 15 else number.gsub(/\s+/, '').length == 16 end end end Similar to how I did with the `number_is_right_length?` method, I’ll now write a test for just the `expired?` method in isolation. The main reasons for extracting code into a smaller method are a) smaller methods are easier to understand, b) smaller methods can more easily be given descriptive and appropriate names, further aiding understanding, and c) it’s more manageable to write tests for small methods with few dependencies than large methods with many dependencies. Finding and Fixing the Bug This test will say, “When the expiration date is in the past, this method should return false.” require 'rspec' require './credit_card' describe CreditCard do describe '#valid?' do context 'expired' do it 'is not valid' do cc = CreditCard.new('3843111122223333', '02/17', 'Visa') expect(cc).not_to be_valid end end end describe '#number_is_right_length?' do context 'no spaces' do context 'right length' do it 'returns true' do valid = CreditCard.number_is_right_length?('Visa', '3843111122223333') expect(valid).to be true end end end end describe '#expired?' do context 'expired' do it 'returns true' do cc = CreditCard.new('3843111122223333', '02/17', 'Visa') expect(cc).to be_expired end end end end If I run this test I notice that it does not pass. I’ll write another test for the opposite scenario: when the expiration date is in the future, the method should return true. require 'rspec' require './credit_card' describe CreditCard do describe '#valid?' do context 'expired' do it 'is not valid' do cc = CreditCard.new('3843111122223333', '02/17', 'Visa') expect(cc).not_to be_valid end end end describe '#number_is_right_length?' do context 'no spaces' do context 'right length' do it 'returns true' do valid = CreditCard.number_is_right_length?('Visa', '3843111122223333') expect(valid).to be true end end end end describe '#expired?' do context 'expired' do it 'returns true' do cc = CreditCard.new('3843111122223333', '02/17', 'Visa') expect(cc).to be_expired end end context 'not expired' do it 'returns false' do cc = CreditCard.new('3843111122223333', '02/30', 'Visa') expect(cc).not_to be_expired end end end end This test also does not pass. So the method returns true when it should return false and false when it should return true. It’s now clear to me where the mistake lies: I put `<` when I should have put `>`! Switching from `<` to `>` fixes the bug and makes both tests pass. require 'date' class CreditCard def initialize(number, expiration_date, brand) @number = number @expiration_date = expiration_date @brand = brand end def valid? if expired? false else self.class.number_is_right_length?(@brand, @number) end end def expired? month, year = @expiration_date.split('/').map(&:to_i) year += 2000 DateTime.now.to_date > Date.new(year, month) end def self.number_is_right_length?(brand, number) if brand == 'American Express' number.gsub(/\s+/, '').length == 15 else number.gsub(/\s+/, '').length == 16 end end end But again, I’m not a huge fan of the fact that I have to bring irrelevant setup data into the picture. Card number and brand have no bearing on whether a date has passed or not. I can give this method the same treatment as `number_is_right_length?` by making it a class method and adding an `expiration_date` parameter. require 'date' class CreditCard def initialize(number, expiration_date, brand) @number = number @expiration_date = expiration_date @brand = brand end def valid? if self.class.expired?(@expiration_date) false else self.class.number_is_right_length?(@brand, @number) end end def self.expired?(expiration_date) month, year = expiration_date.split('/').map(&:to_i) year += 2000 DateTime.now.to_date > Date.new(year, month) end def self.number_is_right_length?(brand, number) if brand == 'American Express' number.gsub(/\s+/, '').length == 15 else number.gsub(/\s+/, '').length == 16 end end end Now I can change my `expired?` tests to only deal with date and nothing else. require 'rspec' require './credit_card' describe CreditCard do describe '#valid?' do context 'expired' do it 'is not valid' do cc = CreditCard.new('3843111122223333', '02/17', 'Visa') expect(cc).not_to be_valid end end end describe '#number_is_right_length?' do context 'no spaces' do context 'right length' do it 'returns true' do valid = CreditCard.number_is_right_length?('Visa', '3843111122223333') expect(valid).to be true end end end end describe '#expired?' do context 'expired' do it 'returns true' do expect(CreditCard.expired?('02/17')).to be true end end context 'not expired' do it 'returns false' do expect(CreditCard.expired?('02/30')).to be false end end end end Future-Proofing Lastly, this most recent test has an unacceptable flaw. I’m checking that the card is not expired if the date is February 2030. As of 2018, February 2030 is in the future, but what happens if this program survives until March 2030? My test suite will break even though the code still works. So instead of making the dates hard-coded, let’s make them relative to the current date. require 'rspec' require './credit_card' describe CreditCard do describe '#valid?' do context 'expired' do it 'is not valid' do cc = CreditCard.new('3843111122223333', '02/17', 'Visa') expect(cc).not_to be_valid end end end describe '#number_is_right_length?' do context 'no spaces' do context 'right length' do it 'returns true' do valid = CreditCard.number_is_right_length?('Visa', '3843111122223333') expect(valid).to be true end end end end describe '#expired?' do let(:this_year) { Date.today.strftime('%y').to_i } context 'expired' do it 'returns true' do expect(CreditCard.expired?("02/#{this_year - 1}")).to be true end end context 'not expired' do it 'returns false' do expect(CreditCard.expired?("02/#{this_year + 1}")).to be false end end end end The Current State of the Code Here’s what our class looks like right now: require 'date' class CreditCard def initialize(number, expiration_date, brand) @number = number @expiration_date = expiration_date @brand = brand end def valid? if self.class.expired?(@expiration_date) false else self.class.number_is_right_length?(@brand, @number) end end def self.expired?(expiration_date) month, year = expiration_date.split('/').map(&:to_i) year += 2000 DateTime.now.to_date > Date.new(year, month) end def self.number_is_right_length?(brand, number) if brand == 'American Express' number.gsub(/\s+/, '').length == 15 else number.gsub(/\s+/, '').length == 16 end end end This code actually doesn’t look super great. It’s arguably not even better than what we started with. Here are some things we could do to improve it:

Change the `if/else` in `valid?` to use a guard clause

DRY up the length checks in `expired?`

Add test coverage for everything that’s not covered yet—which is a lot

Put the “magic numbers” of 15 and 16 into constants

I also am not sure I’m a huge fan of the class methods. What would it look like if we changed those back to instance methods?

The Fully Refactored Version

Here’s what I came up with after further refactoring:

require 'date' class CreditCard NUMBER_LENGTH = 16 NUMBER_LENGTH_AMEX = 15 def initialize(number, expiration_date, brand) @number = number @expiration_date = expiration_date @brand = brand end def valid? number_is_right_length? && !expired? end def expired? DateTime.now.to_date > Date.new(*expiration_year_and_month) end def expiration_year_and_month month, year = @expiration_date.split('/').map(&:to_i) [year + 2000, month] end def number_is_right_length? stripped_number.length == correct_card_length end def correct_card_length if @brand == 'American Express' NUMBER_LENGTH_AMEX else NUMBER_LENGTH end end def stripped_number @number.gsub(/\s+/, '') end end

In addition to the bullet points above, I made some methods smaller by pulling certain functionality out into new ones.

Here’s the final test suite:

require 'rspec' require './credit_card' describe CreditCard do let(:this_year) { Date.today.strftime('%y').to_i } let(:future_date) { "02/#{this_year - 1}" } let(:past_date) { "02/#{this_year + 1}" } describe '#valid?' do context 'expired' do it 'is not valid' do cc = CreditCard.new('1111111111111111', '02/17', 'Visa') expect(cc).not_to be_valid end end end describe '#number_is_right_length?' do context 'no spaces' do context 'right length' do it 'returns true' do cc = CreditCard.new('1111111111111111', future_date, 'Visa') expect(cc.number_is_right_length?).to be true end end end context 'American Express' do it 'needs to be 15 numbers long' do cc = CreditCard.new('111111111111111', future_date, 'American Express') expect(cc.number_is_right_length?).to be true end end context 'non-American-Express' do it 'needs to be 16 numbers long' do cc = CreditCard.new('1111111111111111', future_date, 'Visa') expect(cc.number_is_right_length?).to be true end end end describe '#expired?' do context 'expired' do it 'returns true' do cc = CreditCard.new('1111111111111111', future_date, 'Visa') expect(cc).to be_expired end end context 'not expired' do it 'returns false' do cc = CreditCard.new('1111111111111111', past_date, 'Visa') expect(cc).not_to be_expired end end end end