Andrew Glover (aglover@stelligent.com), 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.

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:

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:

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:

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

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:

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

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

• "In pursuit of code quality: Monitoring cyclomatic complexity" (Andrew Glover,
  developerWorks, March 2006): Learn the simple code metrics that let you
  spot and correct risky code.
  
  
• "In pursuit of code quality: Code quality for software architects" (Andrew Glover,
  developerWorks, April 2006): Use coupling metrics to support your
  system architecture.
  
  
• 
  In pursuit of code quality: Read the complete series by Andrew Glover.
  
  
  
• "Diagnosing Java code: Design for easy code maintenance" (Eric Allen,
  developerWorks, January 2003): Discusses the craft of writing easily maintainable code and suggests tools for
  refactoring.
  
  
• "Testing legacy code"
  (Elliotte Rusty Harold, developerWorks, April 2006): What to do when you stumble across legacy code that's never been tested.
  
  
• "Zap bugs with PMD"
  (Elliotte Rusty Harold, developerWorks, January 2005): Use PMD's built-in rules and your own custom rule sets to improve the quality of your Java code.
  
  
• Refactoring: Improving the Design of Existing Code (by Martin Fowler; Addison Wesley, 1999): The essential introduction to refactoring patterns.
  
  
• The Java technology zone: Hundreds of articles about every aspect of Java programming.

• JUnit-addons: A useful library for anyone using
  JUnit.
  
  
• PMD: Scans Java code
  for problems.
  
  

• Participate in the discussion forum.
  
  
• developerWorks
  blogs: Get involved in the developerWorks community!

About the author

		Andrew Glover is the 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. He is the co-author of Java Testing Patterns (Wiley, September 2004).