Taming legacy Ruby code using the “Sprout Method” technique (example 1)

by Jason Swett,

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
  4. 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
  •  
  •  
  •  
  • 10

6 thoughts on “Taming legacy Ruby code using the “Sprout Method” technique (example 1)

  1. Mike

    Aren’t these the wrong way around:

    let(:future_date) { “02/#{this_year – 1}” }
    let(:past_date) { “02/#{this_year + 1}” }

    Won’t 1 be 1 less than the year, so last year? You use future_date to make a valid card, so I assume wrong way around?

    Reply
      1. Mike

        Great post BTW. One other small change, given you are using lets, this line seems risky:

        cc = CreditCard.new(‘1111111111111111′, ’02/17’, ‘Visa’)

        Either a future or past date rather than a hard coded date makes more sense, no?

        Reply
        1. Jason Swett Post author

          Thanks, and good point. In this case the 02/17 is safe because it will always be in the past, although something like PAST_DATE = ’02/17′ for clarity might have been better.

          Reply
  2. Tachyons

    One potential downside is you are exposing methods other than valid? to outside and test is closely coupled to the implementation details

    Reply

Leave a Reply to Harsh Deep Cancel reply

Your email address will not be published. Required fields are marked *