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
endWhy 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?
=> trueIf 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
- 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
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
endWhen 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
endI’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
endNow 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
endIf 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
endIn 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
endExtracting 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
endThat 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
endSimilar 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
endIf 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
endThis 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
endBut 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
endNow 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
endFuture-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
endThe 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
endThis 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:
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
endIn 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