Last month I submitted my fourth article from the 'Code Cop' series. After
Daily Build, Daily Code Analysis and
Automated Testing I wrote about
checking architecture rules. One approach described there used
PMD. In case you don't know what PMD is, it's a static code analysis tool for Java. It was first developed in 2002 and is updated regularly. It checks the abstract syntax tree (AST) of Java code. (PMD is a great tool. You should definitely use it!)
PMD vs. AST (I just love TLAs)
PMD comes with many predefined rules. Some of them are real life-savers, e.g.
EmptyCatchBlock. It's possible to define your own rules. The easiest way to do this is to use
XPath expressions to match patterns in the AST. For example, the following trivial class
package org.codecop.myapp.db;
import java.sql.Connection;
public class DBSearch {
...
}is represented inside PMD by the following AST
+ PackageDeclaration
| + Name:org.codecop.myapp.db
+ ImportDeclaration
| + Name:java.sql.Connection
+ TypeDeclaration
+ ClassOrInterfaceDeclaration:DBSearch
+ ClassOrInterfaceBody
+ ...An XPath expression to find imports from the
java.sql package looks like
//ImportDeclaration[starts-with(Name/@Image, 'java.sql.')]
It is quite simple (at least when you are familiar with XPath ;-) For further details read the
PMD XPath Rule Tutorial.
I've always been enthusiastic about static code analysis and PMD in particular and have been using it successfully since 2004. (For example see my very first presentation on
static code analysis with PMD.) I love it; It's a great tool. I have created several custom rules to
enforce consistent coding conventions and to prevent bad coding practices. Today I am going to share some of these rules with you in this post.
First Rule
One common bug is
public boolean equals(MyClass o) instead of
public boolean equals(Object o) which shows that the developer is trying (and failing) to override the
equals(Object) method of
Object in
MyClass. It will work correctly when invoked directly with a
MyClass instance but will fail otherwise, e.g. when used inside collections. Such suspiciously close (but still different)
equals methods are matched by
//ClassDeclaration//MethodDeclarator
[@Image = 'equals']
[count(FormalParameters/*)=1]
[not ( FormalParameters//Type/Name[
@Image='Object' or @Image='java.lang.Object' ] ) ]
This rule explained in plain English matches all the method declarations that are named
equals, and have one parameter which type is neither
Object nor
java.lang.Object. (
Object is mentioned twice because PMD analyses the source and therefore can't know about simple and full qualified class names.) Later, Tom included this
SuspiciousEqualsMethodName rule into PMD's default set of rules.
Copy and Paste
The easiest way to define your own rule is to take an existing one and tweak it. Like
SuspiciousEqualsMethodName was derived from
SuspiciousHashcodeMethodName, the next
JumbledIterator is quite similar to
JumbledIncrementer. (
JumbledIterator was created by Richard Beitelmair, one of my colleagues who appointed me "Code Cop" and presented me with my
first Code Cop T-shirt.) So what's wrong with the following line?
for (Iterator it1 = iterator(); it2.hasNext(); ) { ... }Most likely
it2 should be
it1, shouldn't it. Richard created the following rule to pick up on these kinds of errors:
//ForStatement[
( ForInit//ClassOrInterfaceType/@Image='Iterator' or
ForInit//ClassOrInterfaceType/@Image='Enumeration'
) and
( ends-with(Expression//Name/@Image, '.hasNext') or
ends-with(Expression//Name/@Image, '.hasMoreElements')
) and not (
starts-with(Expression//Name/@Image,
concat(ForInit//VariableDeclaratorId/@Image, '.'))
)
]A Real Environment
After the last two sections we are warmed up and finally ready for some real stuff. On several occasions I have met developers who wondered why their code
Long.getLong(stringContainingANumber) would not work. Well it worked, but it did not parse the String as they expected. This is because the
Long.getLong() is a shortcut to access
System.getProperty(). What they really wanted was
Long.parseLong(). Here is the
UnintendedEnvUsage rule:
//PrimaryExpression/PrimaryPrefix/Name[
@Image='Boolean.getBoolean' or
@Image='Integer.getInteger' or
@Image='Long.getLong'
]
Care for Your Tests
PMD provides
several rules to check JUnit tests. One of my favourite rules is
JUnitTestsShouldIncludeAssert which avoids (the common) tests that do not assert anything. (Such tests just make sure that no
Exception is thrown during their execution. This is fair enough but why bother to write them and not add some
assert statements to make sure the code is behaving correctly.) Unfortunately, one "quick fix" for that problem is to add
assertTrue(true). Rule
UnnecessaryBooleanAssertion will protect your tests from such abominations.
A mistake I keep finding in JUnit 3.x tests is not calling
super in test fixtures. The framework methods
setUp() and
tearDown() of class
TestCase must always call
super.setUp() and
super.tearDown(). This is similar to constructor chaining to enable the proper preparation and cleaning up of resources. Whereas
Findbugs defines a rule for that, PMD does not. So here is a simple XPath for
JunitSetupDoesNotCallSuper rule:
//MethodDeclarator[
( @Image='setUp' and count(FormalParameters/*)=0 and
count(../Block//PrimaryPrefix[@Image='setUp'])=0
) or
( @Image='tearDown' and count(FormalParameters/*)=0 and
count(../Block//PrimaryPrefix[@Image='tearDown'])=0
)
]Obviously this expression catches more than is necessary: If you have a
setUp() method outside of test cases this will also be flagged. Also it does not check the order of invocations, i.e.
super.setUp() must be the first and
super.tearDown() the last statement. For Spring's
AbstractSingleSpringContextTests the methods
onSetUp() and
onTearDown() would have to be checked instead. So it's far from perfect, but it has still found a lot of bugs for me.
That's all for now. There are more rules that I could share with you, but this blog entry is already far too long. I've set up a repository
pmd-rules containing the source code of the rules described here (and many more).
The quality of the presentations was excellent. I really liked all of them. This was the most important thing to me because I went there to see the presentations. I didn't care whether the Wi-Fi was slow or if the food was not particularly tasty. I attend conferences to see new things. (Well the food was nothing special, but who cares?) I have attended other conferences before and there were usually a few presentations that were boring or not very good. However, this was not the case with GeeCON! I was pleasantly surprised. Kudos to the GeeCON team and all the speakers. You did a great job.
