FindBugs

Inspired by Josh Bloch and Bill Pugh’s Java Puzzlers talk at Google, Java Puzzlers, episode VI, I decided to use FindBugs and analyze some core Java libraries we wrote and used at one of my previous employments. Here are some of the findings:

Commons: 675 classes, 505 bugs (98 bad practice, 27 correctness, 96 malicious code vulnerability, 14 multithreaded correctness, 207 performance, 63 dodgy).

Messaging: 31 classes, 21 bugs (9 bad practice, 8 malicious code vulnerability, 3 performance, 1 dodgy)

Services: 239 classes, 78 bugs (5 bad practice, 4 correctness, 35 malicious code vulnerability, 1 multithreaded correctness, 26 performance, 7 dodgy)

Content management: 637 classes, 577 bugs (38 bad practice, 14 correctness, 72 malicious code vulnerability, 3 multithreaded correctness, 382 performance, 68 dodgy)

Example bugs include:

Bad attempt to compute absolute value of signed 32-bit hashcode:

indexPrimary +=  Math.abs(this.getStoragePrimary().hashCode()) + SEP;

Why? If the hash code is equal to Integer.minValue() then the result will be negative as well.

Impossible cast (ouch!):

ArrayList list=new ArrayList();
setChoices((DateDatum[])list.toArray());

Possible null reference dereference for internalConnetion:

try {
  if(internalConnetion == null) {
    throw new TransactionManagerException("...");
  }
  ...
  } catch(Exception e) {
    throw new TransactionManagerException(
      e.getMessage(), e);
  } finally {
    if (internalConnetion.getDepth() < 1) {

Null check of value previously de-referenced:

if (accountId.equals(internalAccount)) {
  permissions.add(new AllPermission());
  return permissions;
}
if (accountId == null) {
  return permissions;
}

This last comparison for null is redundant since, if true, it would have already raised an exception.

Method invokes inefficient Boolean constructor:

return new Boolean(false);

Boolean objects are immutable, there’s no need to create a new instance; use Boolean.valueOf(...) instead.

Method invokes inefficient new String(String) constructor:

String path = new String("");

Method concatenates strings using + in a loop:

for(int i = 0; i < (hash.length / 2); i++) {
  rtnValue += Integer.toHexString(x);
}

Inefficient use of keySet iterator instead of entrySet iterator:

Set keySet = headers.keySet();
Iterator iterator = keySet.iterator();
while(iterator.hasNext()) {
  String value = (String) headers.get(key);
}

May expose internal representation by incorporating reference to mutable object:

public void setMethods(Hashtable methods) {
  this.methods = methods;
  FunctionsContainer.getLogger().info("Loaded " +
    methods.size()+" methods");
}

This code stores a reference to an externally mutable methods object into the internal representation of the object. Storing a copy of the object would have been much safer.

It’s always a good idea to statically analyse your code once you’re done with it. It’s not going to render it bug free but it certainly helps. Oh, and while I’m at it, go get yourself a copy of Java Puzzlers book; it helps avoiding some very dark corners you might not have been aware of.