Characterizing code for refactoring

Changing untested production code without unwanted side-effects

The primary purpose of writing characterization tests is to protect business functionality from inadvertent side-effects when difficult-to-change code needs to be modified. Untested or poorly tested legacy code can be complex, difficult to read, and not injectible — all of which makes it hard to change when you need to. This article explains how to characterize such code properly for unit testing so that you can refactor it safely.

Share:

B.J. Allmon (bjallmon@gmail.com), Director, Practice & Product Delivery, IQ Innovations (iQity)

B.J. Allmon is a seasoned agile software craftsman and the director of practice and product delivery at IQ Innovations. He's also an author, speaker, songwriter, and the founder of the nonprofit Experience Foundation, which provides apprenticeship opportunities and hands-on experience to those with little to no background in software development. You can reach B.J. on Twitter @bjallmon.



18 December 2012

Also available in Russian

When you need to add a new feature or fix a bug in legacy source code, it's not uncommon to find that you're working with confusing, carelessly written code. It often lacks readability, and its logic might even betray some confusion on the original programmer's part about the problem it was intended to solve. What's worse, many times this kind of code is poorly tested or not tested at all.

It's important not to change any production code until you have sufficient unit tests in place. By starting with characterization tests, you re-create the code's current behavior and gain a change-enabling pivot point. With characterization tests in place, you can then use test-driven development (TDD) to refactor the code and finish the job.

With the help of examples that use the Grails framework, this article shows when and how to characterize code with automated unit tests for refactoring. This process enables a team to change software with greater precision as it receives guidance from TDD along the way.

Beware the quick fix

I'll set the stage with a mock scenario. Suppose you're a new developer for a company that has started using a tracking system for the various software products it builds and supports. The system tracks a variety of things: features, technical debt, production defects, prerelease bugs, and support-type items. The company calls these elements stories.

Various statuses are available per story that facilitate feedback. The stories and their status also enable a visual Kanban-style electronic cardwall. The seven available statuses that drive the Kanban segments are supposed to start with New and end with Ship It!. And that's where the problem begins — a problem with the tracking system itself.

Someone has created a new defect entry that describes an issue with the system's available statuses. It appears that the Ship It! status is missing for the Feature story type, as shown in the cardwall in Figure 1:

Figure 1. The feature item is missing the story status
Screenshot of the hypothetical feature-tracking system in which the feature item is missing the Ship It! status

On the system's form page for editing a story, a question mark in angle brackets (<?>) appears instead of the Ship It! choice in the list of available statuses, as you can see in Figure 2:

Figure 2. The form contains a <?> when it should say "Ship It!"
The form contains a question mark when it should say Ship It

The Ship It! status, then, is missing from both the list view and the edit form. When you're asked to investigate, you quickly discover that the issue is inside the StoryStatus enum, shown in Listing 1:

Listing 1. The StoryStatus enum
package com.experiencefoundation.stories

public enum StoryStatus {

    NEW,
    TODO,
    IN_PROGRESS,
    DONE_ISH,
    QA,
    STAGING,
    SHIP_IT

    public String friendlyName() {
        if("NEW".equals(name())) {
            return "New"
        } else if("TODO".equals(name())) {
            return "Todo"
        } else if("IN_PROGRESS".equals(name())) {
            return "In Progress"
        } else if("DONE_ISH".equals(name())) {
            return "Done'ish"
        } else if("QA".equals(name())) {
            return "QA"
        } else if("STAGING".equals(name())) {
            return "Staging"
        } else if("SHIP_IT!".equals(name())) {
            return "Ship It!"
        } else {
            return "<?>"
        }
    }

}

As you can see in Listing 1, the defect results from trying to match on the hard-coded value of SHIP_IT! instead of the enum value's name, SHIP_IT. The hard-coded value includes an exclamation mark (!), and the name doesn't, which causes the application to fall into the final return, the else block. As a result, the friendlyName() method returns <?> whenever the StoryStatus is a StoryStatus.SHIP_IT instance.

Listing 1 also has several general "ugly code" issues:

The quick fix is from the devil

The book Practices of an Agile Developer by Venkat Subramaniam and Andy Hunt (see Resources) says that the quick fix is like quicksand: The clarity of code goes down and confusion is harvested in its place. A better solution is to:

  • Get a good understanding of the architecture and design.
  • Understand the code you modify.
  • Avoid quick hacks.
  • Fix the problem, not the symptom.
  • Don't code in isolation. (For example, use code reviews.)
  • Unit test.

The outcome of following these steps is profoundly positive and enables higher team velocity and clean code.

  • The friendlyName() method contains a great deal of cyclomatic complexity instead of simply using the power of the enum. (The method could even just use a simple map technique, if the code were outside of an enum.)
  • The code favors duplication over using a method with arguments — again, failing to use the power of the enum.
  • The many return statements make for a lot of reading.
  • A method like this should take fewer than five lines of Groovy code; as written, it's unnecessarily long.
  • The code contains quite a large number of hard-coded values. Hard-coded values and magic-number infestation cause confusion in big applications and can be dangerous, especially without tests.

Even in this rather trivial example, it's impossible to know whether any other application elements depend on the SHIP_IT status to provide a return value in a broken state. In many complex cases, this uncertainty is usually a guarantee that just hacking in the "fix" could potentially break something else.

Assuming a lack of existing tests that would provide feedback if something breaks, you had better not change the current paradigm just yet. You don't have enough information yet to know if your changes would cause a side-effect. Enter characterization tests.


Characterizing code: Get rolling

Level of abstraction

Through code inspections, you determine the proper level of abstraction at which to start writing characterization tests. How does the code interact with the rest of the application? Maybe several controllers call some server-side code that you need to change. This would mean that you would want to have some coverage at the controller layer to ensure that those features still work as intended.

If the issue were found to be functionally wrong in the UI, you would consider writing a functional test to correct it. Many options for tools are available, including Selenium, Watir, Cucumber with Watir, HtmlUnit, and Geb. If you find a problem in JavaScript, consider using Jasmine for unit testing JavaScript directly. You could also consider a mixture of testing tools to provide adequate test coverage to the greater story you care about. Regardless, you ultimately must decide what is feasible. (See Resources for links to more information about the testing tools mentioned here.)

One way to get rolling with characterization tests is to search for references to, or uses of, the broken code that you need to change. The search results can provide an architectural picture (story) of how the code is currently used. The uses you find also provide you with the highest level of abstraction that characterization tests should cover. In other words, the search helps to determine where you should start writing unit tests. And it tells you what kinds of tests are needed to ensure the greatest success while providing the best coverage possible. The tests will be asserting what the current behavior is doing, not what it should do. Meanwhile, the current behavior should not change until you have the proper feedback in place; then you can write tests for the purpose of actually refactoring the code.

Thankfully, after searching references using your tooling or IDE of choice, the only uses you find in the StoryStatus example are in a few UI pages and in the Story class itself. The UI pages display the friendly name that the StoryStatus enum provides and nothing more. (However, if you have no functional tests for these view pages, you should definitely create them.

Here's an example snippet of what you would see in the Story create and edit forms:

<g:select name="storyStatus" from="${StoryStatus?.values()*.friendlyName()}"
keys="${StoryStatus?.values()*.name()}" value="${storyInstance?.storyStatus?.name()}"/>

As you can see, the storyStatus select tag uses the friendlyName() values from the StoryStatus enum directly only for display purposes. The <g:select .../> tag is a Grails tag library.

At this point, I would feel good about adding some test coverage for the enum to provide feedback to those who might unintentionally break things in the future. The test coverage would, ideally, prevent any defects stemming from the enum code from ever leaving the development stage and entering production. (Of course, if a team doesn't run the automated tests or keep up with them, there's no promise that they'll be effective.)

The tests written for the enum provide feedback when you make any changes to the enum's values or size. This feedback forces you to change the test coverage expectations when necessary and when the automated tests run. Adding test coverage for special methods like friendlyName() will also be beneficial, as shown in Listing 2:

Listing 2. StoryStatusTests provides the coverage needed
package com.experiencefoundation.stories

import org.junit.Test

class StoryStatusTests {

    @Test
    public void validateStoryStatusSizeAndValues() {
        def expectedValues = [
                StoryStatus.NEW,
                StoryStatus.TODO,
                StoryStatus.IN_PROGRESS,
                StoryStatus.DONE_ISH,
                StoryStatus.QA,
                StoryStatus.STAGING,
                StoryStatus.SHIP_IT
        ]
        assert expectedValues == StoryStatus.values()
    }

    @Test
    public void returnsFriendlyNameForStoryStatus() {
        def expectedValues = [
                "New",
                "Todo",
                "In Progress",
                "Done'ish",
                "QA",
                "Staging",
                "<?>"
        ]
        assert expectedValues == StoryStatus.values()*.friendlyName()
    }
}

The StoryStatusTests class is testing what is currently happening with the application. Notice that in the returnsFriendlyNameForStoryStatus() test you can see that the expected value after Staging is indeed <?>.

Now that you have passing characterization tests in place, it's time to move on to refactoring. Refactoring enables you to change behavior safely through the practice of TDD. Listing 3 demonstrates how to test for the new behavior:

Listing 3. StoryStatusTests revised for returning "Ship It!"
@Test
public void returnsFriendlyNameForStoryStatus() {
    def expectedValues = [
        "New",
        "Todo",
        "In Progress",
        "Done'ish",
        "QA",
        "Staging",
        "Ship It!"
    ]
    assert expectedValues == StoryStatus.values()*.friendlyName()
}

The updated test in Listing 3 demonstrates that the only thing you need to change is the expectedValues list, which you do by changing <?> to Ship It!.

The test fails, because the result is still in a broken state. You can see the result in Figure 3:

Figure 3. The StoryStatusTests is now red, for a good reason
Screenshot showing output from the StoryStatusTests, which displays a red bar because StoryStatusTests has failed

Now that you have a "good" failure, it's time to move on to fixing the defective code. Listing 4 demonstrates how you fix the code using the power of an enum and constructor injection:

Listing 4. Repairing the code
package com.experiencefoundation.stories

public enum StoryStatus {

    NEW("New"),
    TODO("Todo"),
    IN_PROGRESS("In Progress"),
    DONE_ISH("Done'ish"),
    QA("QA"),
    STAGING("Staging"),
    SHIP_IT("Ship It!")

    private String friendlyName

    public StoryStatus(String friendlyName) {
        this.friendlyName = friendlyName
    }

    public String friendlyName() {
        this.friendlyName
    }

}

Now, if you run the tests again, they'll succeed. You have refactored the code to retrieve a friendly name without the error-prone, ugly code that was there before. You can see the result of fixing the enum in the UI. Figure 4 shows the current list view:

Figure 4. Corrected list view
Screenshot of the correct list views, with the Ship It! story status appearing as it should

Figure 5 shows the current form view:

Figure 5. Corrected form view
Screenshot of the correct form view, with the Ship It! story status appearing as it should

Measuring refactoring results

Plenty of static-analysis tools are available to help you find ugly or duplicate code. Some provide metrics for code coverage, as well. Cobertura is the code-coverage tool that the example application uses. Cobertura is provided with the Grails code-coverage plug-in, and you run it by executing the following command at the command line:

grails test-app -coverage

Figure 6 shows the results:

Figure 6. Code coverage metrics summary via Cobertura
Image showing a code coverage metrics summary via Cobertura

The result is positive. With the test coverage you already had, plus the new unit tests for the StoryStatus enum during the refactoring, Cobertura shows that you now have nearly 100 percent coverage across the board,


Conclusion

Approaching changes to production code with care reduces the quantity of potential defects that can occur as a side-effect — and can even eliminate their occurrence completely. Characterization tests re-create the application's current reality to give you a starting point for changing it. After you run the characterization tests, you're in a position to use TDD to refactor the code for the purpose of fixing a bug or adding a feature.

When you reach the point for changing the behavior, refactor the code for optimal clarity, leaving it better than you found it. The simple code in this article's example changed significantly. You could have merely altered the conditional block to determine the friendly name. Instead, you also took the opportunity to remove complexity, enhance the code's clarity, and made it better overall.

As you add coverage, it's good to get feedback on how you're doing. Code-coverage tools like Cobertura come in handy for revealing untested paths and help you in the refactoring process. Code-coverage static-analysis tools provide metrics that tell you what code the tests have exercised. However, they do not tell you that the tests actually ensured that the behavior was correct. Behavior can only be verified by a human and sustained for correctness through automated unit tests.

Resources

Learn

Get products and technologies

  • Selenium, Watir, and HtmlUnit: These tools enable you to build automated functional tests that run in a browser.
  • Cucumber or Geb: Test drive behavior-driven development by trying out Cucumber or Geb (a Groovy framework).
  • Jasmine: Use TDD on your JavaScript code with Jasmine.
  • Cobertura and Emma: Get feedback on your test coverage by using tools like these.

Discuss

Comments

developerWorks: Sign in

Required fields are indicated with an asterisk (*).


Need an IBM ID?
Forgot your IBM ID?


Forgot your password?
Change your password

By clicking Submit, you agree to the developerWorks terms of use.

 


The first time you sign into developerWorks, a profile is created for you. Information in your profile (your name, country/region, and company name) is displayed to the public and will accompany any content you post, unless you opt to hide your company name. You may update your IBM account at any time.

All information submitted is secure.

Choose your display name



The first time you sign in to developerWorks, a profile is created for you, so you need to choose a display name. Your display name accompanies the content you post on developerWorks.

Please choose a display name between 3-31 characters. Your display name must be unique in the developerWorks community and should not be your email address for privacy reasons.

Required fields are indicated with an asterisk (*).

(Must be between 3 – 31 characters.)

By clicking Submit, you agree to the developerWorks terms of use.

 


All information submitted is secure.

Dig deeper into developerWorks


static.content.url=http://www.ibm.com/developerworks/js/artrating/
SITE_ID=1
Zone=Agile transformation, Java technology
ArticleID=851590
ArticleTitle=Characterizing code for refactoring
publish-date=12182012