Rocket Surgery

Those of you who know me or who have read my earlier posts know that I am a static code analysis fan, and some of my favorite tools are PMD, Checkstyle, FindBugs, and JDepend, for their excellent feedback on the state of the quality of your code. In fact, I regularly use all of them and usually in combination with other tools (such as JUnit, Emma, etc) to gather code metrics.

The reports from each of these tools naturally identify problems — why else would you run them? And once you see a problem, you naturally want to run right out and fix them. The report shows them all, rated by severity, with counts and everything, and you can see the types of errors. It’s just a simple refactoring, right?

Better think twice before activating that refactoring in your IDE.

For one, the severity in those reports generally only consider the potential for program failure caused by the problem — and that’s fine. But one should be very, very careful in mistaking the severity of a problem with the amount of effort it will take to remediate. In fact, there is no indication on these reports about what it takes to fix a problem flagged by static code analyzers.

For example, the PMD rule for violating method naming conventions is set at severity 1 (the highest), as is calling overridable methods from a constructor. Generally, renaming a method is simple, particularly with IDE’s such as Eclipse or NetBeans — if the method is private, renaming the method is trivial. Changing a constructor to stop calling overridable methods is a bit more involved and can become truly problematic. The point is - not all severity 1 problems are the same, and they will not all take the same amount of time to correct, nor will they all take developers of the same skill level to remediate.

Here is another example: A rule found in both PMD and Checkstyle is that package names should be all lowercase. A package name gets flagged in your report. Can you casually assign this problem to a junior developer to go refactor the package name to all lower case?

Answer: maybe - it depends on your development environment. In my client’s development environment, they develop on Windows PC’s and use WinCVS clients to a Solaris-based CVS repository. Windows’ file system is not case-sensitive, but Solaris is. If you simply change the case of the package to all lowercase, WinCVS sees this as a change to an existing directory, but CVS on Solaris sees this as an old directory going away and a new one appearing, and things go downhill from there.

So what’s my point? My point is twofold:

1) Project Managers need to understand the reports that come out of static analysis tools, and they need to understand that not all violations are created equal. Some are trivial (cleaning up Java imports); some are hideous to clean up (package dependancy cycles); and they will take different amounts of time and skill to remediate.

2) Developers need to know about the pitfalls of refactoring - particularly automated refactoring - and senior staff should help junior staff become aware of these hazards. For example, did you know:

• The Eclipse “extract method” refactoring can and frequently does create code that modifies the parameters passed into it– a known antipattern?
• The Eclipse “extract local variable” refactoring over a repeated expression can change the behavior of code if the expression has side effects? (that is, it will keep the side effect from occurring multiple times)
• Renaming things is particularly touchy in systems using declarative frameworks, and that you have to remember to look in special files during the rename operation or you’re toast? If your framework derives names (like the way Spring derives URLs by prepending a known prefix and suffix to web page names), even your IDE will only take you so far. You will simply have to remember to change your config file manually after a rename.
• Refactoring without a thorough suite of automated regression tests that every developer can easily run on demand is a recipe for disaster?

So, by all means refactor - but do so deliberately, knowing the pitfalls and how to avoid the problems.

This entry was posted on Tuesday, July 4th, 2006 at 2:56 pm and is filed under Software development.You can follow any responses to this entry through the RSS 2.0 feed.You can leave a response, or trackback from your own site.