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:

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.

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.”

    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.

    I’m going to extract it into a number_is_right_length? method.

    Now that I have this functionality in its own method, I’ll write a test for just that part of the code, in isolation.

    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:

    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.

    Extracting Expiration-Related Code

    Now I’m going to extract the expiration-related code into its own method. That code is highlighted below.

    That chunk of code will go out of the valid? method and into a new expired? method.

    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.”

    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.

    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.

    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.

    Now I can change my expired? tests to only deal with date and nothing else.


    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.

    The Current State of the Code

    Here’s what our class looks like right now:

    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:

    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:

  • 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?

      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?

        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.

  2. Tachyons

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


Leave a Reply

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