Contents


Characterizing code for refactoring

Changing untested production code without unwanted side-effects

Comments

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

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


Downloadable resources


Related topics

  • Working Effectively with Legacy Code (Michael Feathers, Prentice Hall, 2004): Learn more about refactoring legacy code and TDD by reading this book.
  • Practices of an Agile Developer: Working in the Real World (Venkat Subramanian and Andy Hunt, Pragmatic Bookshelf, 2005): Read about the personal habits, ideas, and approaches of successful agile software developers.
  • Kanban: Kanban helps teams visualize a continuous workflow model.
  • Grails: Learn more about the Grails web development framework.
  • 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).
  • Cobertura and Emma: Get feedback on your test coverage by using tools like these.

Comments

Sign in or register to add and subscribe to comments.

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