You Should Be Using A Static Analysis Tool

In my experience static analysis tools do not provide very much benefit. Nonetheless, you’d be crazy, dare I say unprofessional, not to be using a static analysis tool.

These days I’m coding in Java and I’m using FindBugs as my static analysis tool. There is almost no cost to installing and using FindBugs. Most importantly, FindBugs doesn’t have my particular set of blind spots.

Let’s see what FindBugs discovered about my current codebase, a codebase that has been in production for 4 years.

selectField.getModel().setSelectedItem(
    theDescriptor.isActive() ? ITEM_ASSESSMENT_YES : ITEM_ASSESSMENT_YES);

I resolved the FindBugs discovery by replacing the dodgy code with:

selectField.getModel().setSelectedItem(
    theDescriptor.isActive() ? ITEM_ASSESSMENT_YES : ITEM_ASSESSMENT_NO);

This is a real bug discovered by FindBugs. In the dodgy code above, a checkbox in the UI is always selected without considering the value returned from the isActive() method. In my current codebase, FindBugs found three occurrences of the same mistake. No doubt from the essential but sometimes dangerous practice of cut-and-paste coding.

if (getDiseaseToMeasuresOrder() != null &
    getDiseaseToMeasuresOrder().containsKey(theDisease)) {

I resolved the FindBugs discovery by replacing the dodgy code with:

if (getDiseaseToMeasuresOrder() != null &&
    getDiseaseToMeasuresOrder().containsKey(theDisease)) {

This is a real bug discovered by FindBugs. In the dodgy code above, whenever the getDiseaseToMeasuresOrder() method returns null we’ll get a NullPointerException. I’m pretty sure this bug was simply due to a typo.

AbstractEvent<eventType> event = (AbstractEvent<eventType>) theEvent;
switch (event.getType()) {
    case Init:
        handleInitEvent((InitEvent) event );
    case Indicators:
        handleIndicators((IndicatorsEvent) event );
        break;
}

This code looks like a ClassCastException will be thrown whenever we handle the Init event and then fall through to handle Indicators event.

Now I can either add the missing break;, or I can hunt around to confirm that no one is firing the Init event. I decided to hunt around. As it turns out, the Init event is never fired.

I resolved the FindBugs discovery by replacing the dodgy code with:

AbstractEvent<eventType> event = (AbstractEvent<eventType>) theEvent;
switch (event.getType()) {
    case Indicators:
        handleIndicators((IndicatorsEvent) event );
        break;
}

As an added bonus I can remove the handleInitEvent() method. FindBugs excels at spotting neglected code that should be reworked or eliminated.