Refactoring With Code Metrics

From NeoWiki

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.

Contents 1 What is refactoring? 1.1 Subjective and objective refactoring 1.2 The Extract Method pattern 2 A cure for cyclomatic complexity 2.1 When small is beautiful 3 Testing private methods 3.1 The PrivateAccessor class 4 In conclusion 5 Resources 6 About the author

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.

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

You'll note that the new getStatus() method defined in Listing 2 is declared as private. This creates an interesting testing challenge should you want to verify the behavior of that method in isolation! There are a number of ways to approach this issue:

• Make the method public.
• Make the method protected and put the test case in the same package.
• Make an inner class in the parent class, which is a test case.

It turns out there is another option as well: leave the method as declared (that is, private) and use the nifty JUnit-addons project to test it.

The PrivateAccessor class

The JUnit-addons project has a few handy utilities that facilitate testing with JUnit. One of the most useful of these is the PrivateAccessor class, which makes testing private methods a snap regardless of your testing framework of choice. The PrivateAccessor class has no implicit dependency on JUnit, so you can use it with testing frameworks such as TestNG.

The PrivateAccessor's API is simple -- by providing the name of the method (as a String) and its corresponding parameter types and associated values (in Class and Object arrays respectively) to the invoke() method, the value of the invoked method is returned. Under the covers, the PrivateAccessor class actually turns off object accessibility using the Java Reflection API. Keep in mind, however, that if your VM has custom security settings, the utility may not function correctly.

In Listing 4, I call the getStatus() method with both parameter values set to null. The invoke() method returns an Object, hence the cast to a String. Also note that the invoke() method declares that it throws Throwable, so you either must catch it or let your testing framework handle it, as I've done.

Listing 4. Testing a private method

public void testGetStatus() throws Throwable{
  AccountAction action = new AccountAction();

  String value = (String)PrivateAccessor.invoke(action,
    "getStatus", new Class[]{IStatus.class, List.class},
     new Object[]{null, null});

  assertEquals("should be No Changes Since Creation", 
    "No Changes Since Creation", value);
}

Note that the invoke() method is overridden to either take an Object instance, as in Listing 4, or a Class, in the case that the desired private method is also static.

Keep in mind also that using reflection to invoke private methods does introduce a level of brittleness to the resulting tests. Should someone rename the getStatus() method, the above test will fail miserably; however, if you test frequently, you can quickly make the appropriate fixes.

In conclusion

When battling cyclomatic complexity, keep in mind that the vast majority of paths coded into an application are inherent to its overall behavior. That is, it is quite difficult to dramatically reduce the total number of paths. Refactoring only allows you to push those paths into smaller sections of code, which then become easier to test. Those small sections of code also become easier to maintain.

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.