Custom PMD Rules

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