Some Findbugs detectors

At our company, we've been creating various static analysis rules using PMD, Findbugs, and a home-grown tool all aggregated into a Sonar dashboard. It's pretty cool.

Here are some of the rules that we created and why we chose each particular tool:

Findbugs



Anemic Domain Modeling


Sounds pretty lofty, I know, but I think that we found a decent strategy that catches 80% of these issues.

To detect the practice of Anemic Domain Modeling, it seemed best to look from the point of view of the service provider where the logic usually is coded. We wanted to look for something like this:

public double getOrderTotal(Order o) {
double total = 0.0;
for ( OrderItem oi : o.getItems() ) {
total += oi.getQuantity() + oi.getPrice();
}
return total;
}

(By the way, it isn't my purpose here to debate why Anemic Domain Modeling might, indeed, be an anti-pattern. Read the article referenced to get started on that.)

What we noticed is that in these simple cases 1) no service state was being referenced and 2) no exit point other than those that originated from the method parameter was being invoked. So, our bug checks for these two cases and, finding both, logs a bug.

The reason for Findbugs here over PMD has to do with Findbugs, operating on the Java bytecode, has finer-grained access to the data types of each reference. This was the most common reason for choosing Findbugs over PMD when we did.


Unsaved Object Operation Detector


One of the bugs that has bitten me in the past is when I use an API that appears to mutate the calling object, but doesn't. Consider BigInteger, for example:

BigInteger original = BigInteger.ONE;
original.add(BigInteger.ONE);
// what is the value of 'original'?

The value of 'original' is still one because BigInteger's are immutable. In other words, and method call on BigInteger that returns a BigInteger is actually returning a new instance and not the original modified. This happens with String and several other classes.

To check for this, we currently maintain a list of API's that exhibit this behavior and then detect when a method that returns a value is called and whose return value is not set in the calling code, just as above.

Findbugs was used here because, by using the byte code, it can derive the data type of a reference after it has been declared. PMD can't do this (at least not to our knowledge) because it represents the code as an Abstract Syntax Tree, and it turns out to be fairly difficult to infer cross-transverse relationships in a hierarchical representation.


PMD




Avoid Run On Method Statements


The decision here was to try and make code more maintainable and modular by encouraging developers to not use run-on statements:

Object var2 = var1.method1().method2().method3().method4(var3).method5();

The code above has at least two problems. First, if method2 returns null, then it will result in a NullPointerException that is hard to debug. Second, unit tests become difficult to write because now several levels need to be mocked in order to prepare the variable 'var2'. A possible third problem would be that the method containing this line of code will have a much higher RPC making it more prone to faults and more brittle to change. (The reason I think it's only a possible third is because, even if these were separated into 5 different statements, the method would still have this same problem.)

In this detector, then, we find statements that make more than three chained invocations and notify the developer.

We chose PMD for this because in the byte code world, it became very, very difficult to decide when a method invocation was in the same chain. To be able to tell the difference between an invocation chain and not would be to keep track of a symbol table and program stack, which sounded like a lot of work.

Chains can be easily verified when looking at Java syntax, though, which is what PMD does. What took us 12 hours of trying and re-trying in Findbugs took us only 20 minutes in PMD.


Avoid Methods With Same Name As Class


I stole this one from Java Puzzlers. I didn't even know that this was allowable in Java:

public class MyClass {
public void MyClass() {
// body
}
}

You'll notice that the second line appears to be the signature for a constructor, but instead is the signature for a method! (Notice the 'void' keyword. That makes it a method) This would be crazy confusing, I think, if someone actually did it, so we made a rule.

Again, since this was really verifying syntax, PMD was the easiest approach.


Home-grown


I'll talk about our home-grown stuff in another post, because I have a lot of background to give. The basic idea though, is that the overall health of a project isn't limited to the Java code. Ideally, we'd like to take a stab at HTML, Javascript, CSS, Maven dependencies, and more. There didn't appear to be anything out there that analyzed this types of files, so we created one. More next time.

Josh Cummings

"I love to teach, as a painter loves to paint, as a singer loves to sing, as a musician loves to play" - William Lyon Phelps

0 comments: