PMD: mostly good
Tor Norbye has done a writeup on PMD, a tool that looks for common coding errors in your Java source, for example switch statements without default labels or use of the broken double checked locking pattern. It looked interesting, so I installed the plugin into NetBeans and ran it against the source of my Mobile Bollocks MIDlet. As Tor points out, the default set of selected rules is a bit naff in areas, so I followed his recommendation and enabled them all, then started removing the ones I didn't agree with - about 30 in total. PMD is probably the best tool of its type that I have used, however that's not to say it doesn't have a few quirks:
-
It barfs on perfectly OK code. For example it reports the following error:
com.bleaklow.bollocks.ui.About [1]: Error while processing com.bleaklow.bollocks.ui.About; net.sourceforge.pmd.ast.ASTAllocationExpression
This is triggered by code of the form:public class Foo { public Foo() { new Runnable() { public void run() { Foo.this.back(); } }; } public void back() { } }The trigger seems to beFoo.this.back(), I've logged the problem - see this SourceForge bug. -
Some of the rules which claim to catch a particular error don't just catch the error that they claim to, they report errors against perfectly OK code. For example the
SimplifyBooleanExpressionsrule claims it is for problems like this:public class Bar { // can be simplified to // bar = isFoo(); private boolean bar = (isFoo() == true); public isFoo() { return false;} }which I agree is a bad practice, the problem is that it also flags things like this as being errors:if (isFoo() == false) { doSomething(); }which I actually think is clearer than the alternative:if (! isFoo()) { doSomething(); }That!is easy to miss, so I think the explicit comparison againstfalseis actually clearer. -
Some of the rules are just plain stupid - for example insisting that a method contains only a single
returnstatement or prohibiting single-character variable names for things like array indexes. -
Some of the rules are well-intentioned but the implementation is wrong. For example the
AvoidInstansiatingObjectsInLoopsrule is trying to warn you about the potential performance problems that can occur if you continually re-instansiate objects inside a loop, for example:for (i = 0; i < someArray.length; i++) { String s = "some string"; doStuff(someArray[i], s); }the problem is that it complains about common idioms such as this, which are perfectly fine:for (int i = 0; i < someArray.length; i++) { if (matchesCondition(someArray[i]) { matchedThing = new SomeObject(someArray[i]); break; }I guess what it should really be looking for is the instantiation of an object inside a loop that isn't predicated by a conditional statement.
The other problems I have with PMD are more related to missing functionality, and the degree of integration of PMD into NetBeans:
- Although the developer documentation implies the rules can have priorities, they don't appear to be used, at least not in NetBeans. This means that it's difficult to spot rules that have caught real coding errors in amongst a sea of stylistic warnings.
- There is no way to suppress rules once you have manually checked that the code is actually OK. Ideally you would be able to disable rules on both a file and method basis, but you don't seem to be able to do this.
-
You also need to be able to have different rulesets for different projects. For example I got gazillions of errors related to the use of
Vectorinstead of one of theCollectionclasses - this would be a fair warning under normal circumstances, but J2ME doesn't actually provide anything other thanVector, so it would be useful to disable those rules for J2ME projects.
However, even with these provisos it is a great tool and it did find a couple of genuine bugs in my code which my code reviewer cough Gary cough missed ;-)
