Code documentation

Development tools

Code Structure

Techniques and Standards

How To

Functional Info

Background Info

JMRI Code: Static Analysis with SpotBugs

SpotBugs is a tool that can examine code to find possible problems. It does a "static analysis", looking through the code to find certain "known bad ideas": Things that are likely to cause occasional/intermittent problems, poor performance, etc. Because those kinds of problems are hard to find with tests, finding them by inspection is often the only realistic approach. Having a tool that can do those inspections automatically, so that they can be done every time something is changed, keeps the code from slowly getting worse and worse without anybody noticing until it's too late.

For more information on SpotBugs, see the SpotBugs home page.

We routinely run SpotBugs as part of our continuous integration process. You can see the results of the most recent build online here.

If SpotBugs is finding a false positive, a bug that's not really a bug, you can turn it off with an annotation such as:

@edu.umd.cs.findbugs.annotations.SuppressFBWarnings("FE_FLOATING_POINT_EQUALITY","OK to compare floats, as even tiny differences should trigger update")
Note that this should be all on one line. Although Java itself considers it optional, we require the second "justification" argument. Explaining why you've added this annotation to suppress a message will help whoever comes after you and is trying to understand the code. It will also help make sure you properly understand the cause of the underlying bug report: Sometimes what seems a false positive really isn't. Annotations without a justification clause will periodically be removed.

For clarity, this annotation also supports the form:

@edu.umd.cs.findbugs.annotations.SuppressFBWarnings(value = "FE_FLOATING_POINT_EQUALITY", justification = "OK to compare floats, as even tiny differences should trigger update")

This can make it easier to see what is what when quickly scanning through the code.

If you need to put more than one message type in an annotation, use array syntax:

@edu.umd.cs.findbugs.annotations.SuppressFBWarnings("{type1},{type2}","why both are needed")

There are also Java and SpotBugs annotations that can help it better understand your code. Sometimes they'll give it enough understanding of e.g. when a variable can be null, that it'll no longer make false-positive mistakes. For more on this, see the Java annotations and SpotBugs annotation pages.

The basics of annotations are covered in a Java annotations tutorial.

It can be useful to mark code with one of the following annotations so that SpotBugs does a good job of reasoning about it:

For more information about these annotations, please see links above and

We do not use these annotations:

Suppressed Warnings

We have turned off the routine SpotBugs checking of certain kinds of conditions:
This flags cases where a class implements an interface, and also inherits from a superclass that already implements that interface. This is redundant and untidy, but it can't cause the code to malfunction. We have enough of them that we've turned off the warning, and will come back to it some later time.
Static, as opposed to non-static, inner classes (classes defined within another class) take less space because they don't maintain references to the containing object. This warning suggests moving an anonymous (defined in-line to the code) inner class to a regular (defined not in-line) class so it can be made static. Although probably a small improvement, it's a bit of work for a small improvement. We have enough of them that we've turned off the warning, and will come back to it some later time.
What should a method that returns an array of values do when there aren't any? Returning "null", as opposed to an empty array, requires all the calling code to handle a special case. In many cases, returning an empty array makes the code simpler. But for existing code, that simplification is marginal. As this is a design issue, but it crops up a lot, we've suppressed these for now just to focus on more pressing problems.
Malicious Code
This is a class of warnings centered around the idea that data and code methods shouldn't be too visible, especially when static. This is true, but JMRI isn't a hardened library that's being released into a world of people trying to break it, as these changes are a lower priority.
The convention is that class names start with a capital letter, and method names (data and code) with a small letter.
This is a large class of warnings associated with Java serialization. JMRI doesn't used serialization, and is unlikely to do so in the future, so we suppress these to raise the average quality of the issued warnings.


Simon White added FindBugs support to our Ant-based build chain during the development of JMRI 2.5.5. His note on this follows.

As per feature request 1716873, I've added an ant task to run SpotBugs. This will produce a report in HTML in the same location as the JMRI jar (i.e. the top level project dir). Note the new task first runs ant dist because SpotBugs examines the jmri.jar file.

To run the task:

On my old machine this takes about 20 minutes but your mileage may vary.

Note that in the build.xml I have set the maximum memory -Xmx setting for the spotbugs task to 1024m. If your system has more memory, boosting this may speed things up.

Running this on JMRI 2.5.4 produced the following:

Bad practice Warnings 164
Correctness Warnings 77
Experimental Warnings 7
Malicious code vulnerability Warnings 221
Multithreaded correctness Warnings 90
Performance Warnings 459
Dodgy Warnings 304
Total 1322

A lot of work has gone into JMRI during the 2.12 release cycle to bring the bug count down to zero. The Continuous Integration server runs SpotBugs twice a day, which helps developers see the results of their coding without having to set up to run SpotBugs themselves.

If you are checking code in to the JMRI repository, please check the CI server and make sure that your changes do not introduce new errors.