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

1. Try to debug the code by tediously running it over and over on the console
2. 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:

1. Write a test around the buggy area—expiration date validation—and watch it fail
2. Extract the expiration date code into its own method so we can isolate the incorrect behavior
3. 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