Refactoring With Code Metrics

From NeoWiki

(Difference between revisions)

Revision as of 13:35, 5 March 2007

On-target refactoring with code metrics and the Extract Method pattern

Andrew Glover, President, Stelligent Incorporated

30 May 2006

In earlier installments of In pursuit of code quality, you learned how to use code metrics to objectively measure code quality. This month, Andrew Glover shows you how to use those same metrics and the Extract Method pattern for targeted refactoring.

In middle school, I had an English teacher who claimed that "writing is rewriting what one has already rewritten." It wasn't until college that I actually took his words to heart. Not surprisingly, once I consciously embraced this practice, I started to enjoy writing. I started to take pride in what I was writing. I began to put real effort into how the words flowed and the meaning I was trying to convey.

When I began my career as a developer, I would read technical books authored by seasoned professionals and wonder why they spent so much time writing about code. At the time, coding seemed like such an easy job -- someone (invariably senior to me) would give me a problem and I would solve it any way I could.

It wasn't until I started working on large projects with other developers that I began to understand what taking my craft seriously meant. It was at this point in my career that I started to consciously care about the code I was writing, and even to care about the code others were writing. I now knew that if I didn't pay attention to code quality, at some point there would be a late night where I'd be fishing through a mess of spaghetti looking for the proverbial bad meatball.

My ah ha! moment came in late 1999 when I read Martin Fowler's seminal book, Refactoring: Improving the Design of Existing Code, which cataloged a series of refactoring patterns and established, thereby, a common vocabulary for refactoring. Up until then, I had been refactoring my code (and even other's code) without knowing it. I now started to take even more pride in the code I was writing and even in what I was refactoring, for I was putting real effort into how the code flowed and how it could become more maintainable over time.

What is refactoring?

In my opinion, refactoring is the act of improving code that has already been improved. In essence, refactoring is a neverending code-editing process that attempts to enhance the maintainability of a body of code through structural improvements, without changing the code's overall behavior. It's important to remember that refactoring is markedly different from rewriting code.

Rewriting code alters the behavior and even the contract of code, while refactoring preserves its outward interface. To a client of a refactored method, there is no perceivable difference. Things work as they did before, albeit better, most often because of enhanced testability or an apparent performance improvement.

Subjective and objective refactoring

The question then becomes How do I know when I should refactor? The maintainability of a piece of code is arguably a matter of subjectivity. Most of us would, however, find it far easier to maintain something we'd written than to maintain someone else's code. But therein lies the rub -- maintaining your own code throughout your career is a challenging proposition at best. There are few true "code cowboys," lucky enough to travel from job to job, never having to maintain anyone else's code. For most of us, having to maintain someone else's code is just part of being a programmer. The means of deciding whether code should be refactored is often subjective.

It's also possible to objectively determine whether code should be refactored, however, whether it's yours or someone else's. In previous articles in this series, I've shown you how to use code metrics to objectively measure code quality. In fact, you can use code metrics to easily spot code that might be difficult to maintain. Once you've objectively determined there's a problem in the code, you can use a handy refactoring pattern to improve it.

Tip: Always run a test case!
The trick to refactoring code you haven't authored is to do so without making it worse. One thing I learned early in my refactoring journey was the importance of having a test case before I changed anything. I learned this lesson the hard way one night as I fished through my own well-laid-out collection of refactored methods, only to discover that I'd inadvertently broken someone else's working code by attempting to refactor it without a corresponding test case. Please heed my warning and always run a test case before you refactor!

The Extract Method pattern

In the years since Martin Fowler's book was published, many new refactoring patterns have been cataloged; however, by far the easiest pattern to learn, and arguably still the most effective one, remains Extract Method. In this pattern, a logical portion of a method is surgically removed and given its own method definition. The body of the method removed is now replaced with a call to the newly defined method, as demonstrated in the UML diagram shown in Figure 1:

Figure 1. The Extract Method pattern in action

The Extract Method pattern provides two key benefits:

The original method is now shorter and conceivably easier to comprehend.
The body of logic removed and placed into its own method is now easier to test.

A cure for cyclomatic complexity

As it turns out, Extract Method is an excellent dose of medicine for a method that has been infected with a high cyclomatic complexity value. As you may remember, cyclomatic complexity measures the number of paths through a method; therefore, it stands to reason that if you extract some of those paths, the overall complexity of the refactored method will be less.

For example, imagine that after running a code analysis tool like PMD, the resulting report indicates that a class contains a method with a high cyclomatic complexity value, as shown in Figure 2:

Figure 2. A cyclomatic complexity value of 23!

Upon a close visual examination of the method, it turns out that it's unusually long with excessive conditional logic. As I pointed out earlier in this series (see Resources), this increases the risk of defects within the method. Thankfully, the updateContent() method is not without test cases. Even though the method has been deemed risky, the tests mitigate some of the risk.

On the other hand, the tests would have had to be exceptionally well written to hit each of the 23 paths in the updateContent() method. In fact, a good rule of thumb would indicate that at least 23 tests should have been written. Moreover, writing a test case that can precisely isolate the eighteenth conditional in the method turns out to be extremely challenging!

When small is beautiful

Whether or not to accurately test the 18th conditional in a long method is a matter of judgment. If that logic embodies true business value, however, then you'll want to test it, in which case you'll get to see the Extract Method pattern at work. Minimizing the risk is a simple matter of breaking up the conditional logic into smaller pieces, thus creating new methods that can be easily tested.

For instance, the following small piece of conditional logic in the updateContent() method creates a status String. As shown in Listing 1, the logic appears easy enough to isolate:

Listing 1. Conditional logic ripe for the extraction

// ...other code above
String retstatus = null;
if ( lastChangedStatus != null && lastChangedStatus.size() > 0 ){
  if ( status.getId() == ((IStatus)lastChangedStatus.get(0)).getId() ){
    retstatus = "Change in Current status";
  }else{
    retstatus = "Account Previously Changed in: " + 
      ((IStatus)lastChangedStatus.get(0)).getStatusIdentification();
  }
}else{
  retstatus = "No Changes Since Creation";
}

// ...more code below

By extracting this bit of logic into a succinct, new method (shown in Listing 2) you've achieved two things: one, you've lowered the overall complexity of the updateContent() method by about 5; two, you've sufficiently isolated the logic so that you easily test it.

Listing 2. Extract Method yields getStatus

private String getStatus(IStatus status, List lastChangedStatus) {
  String retstatus = null;
  if ( lastChangedStatus != null && lastChangedStatus.size() > 0 ){
    if ( status.getId() == ((IStatus)lastChangedStatus.get(0)).getId() ){
      retstatus = "Change in Current status";
    }else{
      retstatus = "Account Previously Changed in: " + ((IStatus)lastChangedStatus.get(0)).getStatusIdentification();
    }
  }else{
    retstatus = "No Changes Since Creation";
  }
  return retstatus;
}

Now you can replace a part of the body of the updateContent() method with a call to the newly created getStatus() method, as shown in Listing 3:

Listing 3. Calling getStatus

// ...other code above
String iStatus = getStatus(status, lastChangedStatus);
// ...more code below

Remember to run the existing tests to verify nothing has broken!

Testing private methods

Resources

About the author

Andrew Glover is president of Stelligent Incorporated, which helps companies address software quality with effective developer testing strategies and continuous integration techniques that enable teams to monitor code quality early and often. Check out Andy's blog for a list of his publications.

"When you have learned to snatch the error code from the trap frame, it will be time for you to leave.", thus spake the master programmer.

Navigation

Search

Toolbox

LANGUAGES