Recently I attended the No Fluff Just Stuff conference again and learned about a free, fantastic static code analyzer for Java, called PMD. It can use be used standalone or even integrated into many popular IDEs like NetBeans or Eclipse. For the curious, I’d tell you what PMD stands for, but no one really knows; worse yet, I can’t stop myself from typing PDM.
PMD has a nifty rule that allows it to locate Unused Local Variables.
Very quickly I was able to walk through our entire code base, identify things that were assigned to, and subsequently not used, and remove them.
Gotcha #0
The first major hit of the day is that you’ll want to do a Clean on your project before you start. Believe it or not, some project building steps can build intermediate .java, files from your master source code. Problem is, if they appear anywhere inside your project’s directory structure when PMD is making the sweep, they get analyzed to. And you don’t want that.
Gotcha #1
It turns out that there’s a reverse ripple effect in performing this kind of code cleanup and operation. After you’ve removed code, you’ll want to make sure you perform subsequent passes until all concerns are removed.
Take for instance this trivial case:
B = A;
C = B;
// C is unused!
What happens is that once you remove C, it turns out B may no longer be used. Remove B, and it’s even possible A may no longer be used either.
Additionally, this can eventually lead to additional Unused Imports, which dealing with those can also decrease build time.
Gotcha #2 — The Real Evil
Normally, this kind of code clean up is absolutely harmless, although there’s one error of omission that a developer can make which will create problems and cause a silent failure.
Here’s a case where a static code analyzer recommends removing a very important line of code:
boolean doSomething(int x) {
// Do something very important with x
return result;
}
...
boolean result = doSomething( x ); // Do Something Important
// result not used
...
If the return value from a method isn’t used, then the static analyzer will assume the method doesn’t need to be called, and it will recommend commenting out the line — causing your program to silently break.
This is not a error on the part of the tool!
The error was actually that of the developer for not checking the return results of the method.
To correct the problem with the source code, the developer has three options:
- Change the method signature to be of type void.
- Throw an exception from the method.
- Check the return value of the called method.
Otherwise, the real error is that the method may attempt to do something, fail, and communicate back to the caller than something went wrong, but the caller blindly trusts that things are okay.
If you’re not going to check a return value, you shouldn’t be incurring the overhead of sending one. If the library writer provided one for a reason, then you should be using it.
Conclusion
When a code analyzer makes a recommendation, ask yourself what implied rules about your code the analyzer is assuming. Rather than blaming the tool, the better solution is fixing the source.
It may be best to comment out, rather than deleting, code that initially seems superfluous.
Finally, after a massive code sweep, run those unit tests.
Yup, in the “gotcha #2” case, PMD will recommend that you remove the unused local variable – which, as you note, may or may not mean that you can safely remove the whole line. But you can change that line to be just “doSomething( x ); ” without fear, since the local variable to which it’s assigned is indeed unused…
I resemble that remark (Gotcha #2)! When you’re writing rush code, you know that someday you’ll want to add exception/error handling, but not today. You need this feature done today – “doSomething()” needs to be refactored into “doneSomething()!” now!
I would humbly suggest that one would not blindly trust a tool that would comment out a line of code which actually calls a method just because the return value isn’t used. Likewise, saving the 3 nanoseconds loading/storing this unused boolean onto a call stack pales in comparison to the 3 hours one could lose tracking down an error in code that worked for months and that no “one” (alive) has touched!
Thank heaven’s for source code control systems!
Well, to be absolutely fair, the tool does not comment out code, nor does it blindly do anything.
It was letting -me- know that the -return value- was not being used. And, since I had been dealing with literally hundreds of cases where that had been done, I erroneously made the bad assumption that this function had no intended side effects.
Had the code simply not attempted to store the return value in the first place, it wouldn’t have even been flagged.
So there’s a big difference in:
doSomething();
and:
boolean neverGonnaUseThis = doSomething();
If a library returns a value, it most likely does so for a reason.