FindBugs

Recently as part of code improvement exercise in my project a used a static code analysis tool called FindBugs. I find the tool very easy to use and its really helpful in detecting potential bugs in the code.

FindBugs is an open source static analysis tool for defect detection. It analyzes code without running it.


Installation
FindBugs can be installed in either of two ways

Downloading standalone FindBugs application
1. Go to http://findbugs.sourceforge.net/downloads.html and download the zip file under the heading FindBugs tool.
2. Now unzip the downloaded file and go to the bin folder and double click on “findbugs.bat” batch file to open the standalone FindBugs UI interface





3. Now click on FileNew Project and a new pop-up dialog comes up.



4. In this dialog specify the “Project name”. Now click on “Add” button adjacent to “Class archives and directories to analyze” and browse to the class folder of the files that you that want to analyze. Similarly click on “Add” button adjacent to “Source directories” and browse to src folder of the classes that you want to analyze. As an example refer to below snapshot



5. Now click on “Finish” button. FindBugs will now recursively start analyzing all the class files within the specified folder and present you the list of bugs in your source code as shown below



Installing FindBugs eclipse plug-in
Please refer to instruction on http://findbugs.sourceforge.net/downloads.html for installing FindBugs eclipse plug-in


Coding mistakes to avoid
Based on inputs from FindBugs here are some common coding errors that developers normally make and can be avoided.


Inefficient use of keySet iterator instead of entrySet iterator

Suppose we have following HashMap

Map myMap = new HashMap();

Now we following three scenario’s for iterating over elements of Map
Iterating over the keys of Map
In this case if we normally use “map.keySet” as shown below

Iterator iterator = myMap.keySet().iterator();
while(iterator.hasNext())
{
  String key = iterator.next();
}

Iterating over values of Map
In this case we normally use “map.values()” as shown below

Iterator iterator = myMap.values().iterator();
while(iterator.hasNext())
{
  String value = iterator.next();
}

Iterating over both key and value
This is the place where we normally make mistakes by doing following

Iterator iterator = myMap.keySet().iterator();
 while(iterator.hasNext())
 {
  //Retrieving key
  String key = iterator.next();
  
  //Accessing the map again for retrieving the 'value'
  String value = myMap.get(key);
 }

A better way is to use “entrySet” instead of “keySet” in this scenario. In this case the “entry” object contains both key and value and we don’t need to access the map twice.

      Iterator<entry<string, string>> iterator = myMap.entrySet().iterator();
  while(iterator.hasNext())
  {
   Entry<string, string> entry = iterator.next();
   //Accessing the key
   String key = entry.getKey();
   //Accessing the value
   String value = entry.getValue();
  }
   

Method invokes inefficient Number constructor; use static valueOf instead
Suppose we have a primitive int variable and we need to create an Integer object out of it. Then we normally do following, which unnecessary creates an object of Integer class.

       int val = 10;
 Integer intObj = new Integer(val);

A better way would be to use the static valueOf method of Integer class. Integer.valueOf(int) allows caching of values to be done by the compiler, class library, or JVM. Using of cached values avoids object allocation and the code will be faster.

     Integer intObj = Integer.valueOf(val);
  

Method concatenates strings using + in a loop
Avoid using following

        String str = "";
  List nameList = new ArrayList();
  Iterator iterator = nameList.iterator();
  while(iterator.hasNext())
   str += iterator.next();
     

Instead of this use StringBuffer/StringBuilder

        StringBuffer str = new StringBuffer();
  List nameList = new ArrayList();
  Iterator iterator = nameList.iterator();
  while(iterator.hasNext())
   str.append(iterator.next()); //better way
     

Possible null pointer dereference
Refer to following code. What if “statusData” is null. Here in code we have a null check before printing our log statement but not before actually dereferencing this object. This is a very common error.

        Vector statusData = MyClass.getTableData();
  if(statusData != null)
  {
   System.out.println("Status Data is not null");
  }
  else
  {
  System.out.println("Status data is null");
  }
  //What is statusData is null??
  for(int i =0; i < statusData.size(); i++)
   System.out.println("Data...." + statusData.elementAt(i));
 }
    

Improper Exception handling
Consider the following method. It throws IOException

      public static void setData(String data) throws IOException
 {
  if(data == null)
   throw new NullPointerException();
  
  //throw IOException for some error condition
  if(outStream == null)
   throw new IOException();
  
  //otherwise...set data
 }
    

Now here is the code segment where we call this method

 try {
  setData(null);
  } catch (Exception e) {
   System.out.println("got exception");
  }
    

There are two mistakes in the way exception handling has been done here
1. We catch “Exception” instead of “IOException” which the method “setData” actually throws. This way unintentionally we are catching runtime exceptions also which we shouldn’t be doing.
A better way would be to catch only those Exceptions which are thrown by the method

               try {
   setData(null);
  } catch (IOException e) {
   System.out.println("got io");
  }
            

2. Second mistake is that we don’t do anything in the catch block. Exceptions should be propagated to the top level i.e. the initial calling method of the trace so that proper handling can be done and this will avoid execution of part of code which may be dependent on the result of the method throwing an exception.