Wednesday, September 06, 2006

FindBugs

Since I have the need to find the most possible amount of bugs in Matrex to come to version 1.0, which is supposed to be stable, I tried FindBugs (http://findbugs.sourceforge.net/) with success.

FindBugs finds possible bugs in your code.
Or better, as th FindBugs site says: "FindBugs looks for bugs in Java programs. It is based on the concept of bug patterns. A bug pattern is a code idiom that is often an error."

FindBugs can be used as a standalone application or as an Eclipse plugin. Since Matrex is an Eclipse project I tried both the installations. The Eclipse plugin is easier for me to use, because I don't need to configure anything, just install it and use it on my project.
That's how it looks:



You can see that the potential bugs are listed in the Problems view on the bottom; Clicking on a bug line, you get a good explanation of the problem in the Bug Details view on the left.


FindBugs does not pretend to be infallible. The idea is to find areas in your code that can potentially produce problems. So it can happen that some of the potential bugs FindBugs found in your project are false positives, i.e. not bugs. When you can see that one of the bugs of FindBugs is, in all cases, a false positive, you can remove it from the list of bugs FindBugs checks, changing the configuration:



As I told, my experience of FindBugs in my project Matrex is positive.
I have to tell that the current version of Matrex is beta, but it was tested several times, so I would not expect to find too many errors.
Findbugs found around 200 possible bugs, which is a considerable number, but not too high to check them one by one.
These are the bugs it found:

  • EI2: public method exposed internal rapresentation: FindBugs found this bug several times in the code of Matrix.
    One of the examples is in the Matrix class.
    The Matrix class is a light wrapper around a double array, that gives to it some features, among them thread synchronization.
    The array can be directly accessed from outside a Matrix object using the getValues method.
    Another method, setValues, has to be used to change the array.
    But some code could potentially access the array using getValues and change its content, causing inconsistencies.
    FindBugs proposes that getValues returns a copy of the array instead the array itself. That's generally a good solution, but cannot be applied to Matrex, which needs high performance, expecially when accessing matrices.
    So in this case all I could do is to add comment to the getValues method to tell how to use it.
    Since all the warning of this kind are not bugs in Matrex and the code cannot be changed to remove them, I remove EI2 from the list of bugs pattern in the configuration.

  • IS2: inconsistent synchronization:
    In some Matrex classes some of the methods was synchronized, some not.
    That is actually very dangerous and I fixed it.

  • MS: is not final but it should be:
    I forgot to declare some static constants as final. I don't find that dangerous because I know what is supposed to be constant in the Matrex code, but if more developers will work on the same code this bug could result in some problems.
    I fixed it.

  • RCN: null check of previously referenced value:
    I was calling a method of an object, then I was checking if the object was null (which is totally meaningless). In this case, I looked at the code and I could see that the object could not possibly be null. So, I removed the check. Nothing dangerous but confusing for someone reading the code.


  • SIC: should be a static inner class (was not static):
    There was some cases in Matrex in which a class had an inner class that was not declared static, even if it could. No danger, but better to have clean code.
    I fixed it.

  • URF: unread field:
    I use the java.util.logging API to trace Matrex. This requires to declare a static log variable in the class that needs to be traced. It happens that sometimes the variable is declared but never used. Again no danger, but better to have clean code.
    I removed the redundant declaration.

These potential bugs, expecially IS2 and RCN, are dangerous. So, I think FindBugs worked well, and I will re-examine the code of Matrex every time I will release a new version of it.

The only thing I can ask for is the possibility to exclude the checking of a potential bug category (like EI2) only for a certain set of classes: it can be that I know that these classes are correct even if FindBugs finds the bug pattern in them, but I still want to check the rest of the code for it.

No comments: