My Daughter Aarvi

Peace

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.

Code quality metric :)

Singleton pattern is evil!

Singleton pattern is one of the most commonly used patterns. Though it has benefits of its own but there are some inherent problems associated with this pattern which I had observed in my recent work.

Scenario1:
Consider following dummy example where we have a class “Dummy”

public class Dummy {
 
 private static Dummy dummyObject;
 private List m_dataList;
 
 private Dummy()
 {
  m_dataList = new ArrayList();
 }
 
 public static Dummy getInstance()
 {
  if(dummyObject == null)
   dummyObject = new Dummy();
  return dummyObject;
 }
 
 public void saveData(String data)
 {
  m_dataList.add(data);
 }
 
 public List getData()
 {
  return new ArrayList(m_dataList);
 }
}

Now we will be using this class like this

  Dummy dummyObj = Dummy.getInstance();
  dummyObject.saveData("some data");

Now consider a scenario where we use object of this class in 2-3 classes for some specific operation only but since we have “static” reference to Dummy object it won’t ever be removed from memory till the lifetime of the product i.e. now we have resident in memory an object of Dummy class which is not frequently used. This makes a clear case of memory leak.

Proposed resolution
We can provide a method for doing a clean-up i.e. releasing all the resources that this object uses. Onus of doing cleanup lies with the user of this class as only that class knows when it is done with this class.


Scenario2:
Consider a dummy class “InitManager” which is one of the many manager classes that are invoked at the starting of let’s say client or server.

public class InitManager 
{
 private static InitManager initMgr;
 
 private InitManager()
 {
  
 }
 
 public static InitManager getInstance()
 {
  if(initMgr == null)
   initMgr = new InitManager();
  return initMgr;
 }
 
 /**
  * This method performs some boilerplate
  * tasks.
  */
 public void performTask()
 {
  //do the initialization task
 }
}

Now let’s suppose that this class is used only during initialization after which it’s not used throughout the execution of the product i.e. we do something like
InitManager initMgrObject = InitManager.getInstance();
initMgrObject.performTask();

Now the object of InitManager will remain in memory during the lifetime of the product as it has “static” reference which will be released only when the application stops execution. This is clear case of memory leak. A better and simpler approach for this is to have static methods and there is no need to create objects in such cases.

public class InitManager 
{
 public InitManager()
 {
  
 }
 
 /**
  * This method performs some boilerplate
  * tasks.
  */
 public static void performTask()
 {
  //do the initialization task
 } 
}

Dabaang : Dialogues which I love

Kamaal karte ho Pandey ji

Kamini se yaad aaya….pandey ji..apki biwi kaisi hain

Hum tumri jaan mein itna ched karenge itna ched karenge…ki.confuse ho jaoge ki..Saas kaha se ley aur pade kaha se..

101 kamino ki bali deke hamare maa baap ne hummein paida kiya hai
Salman Khan: unhi kamino ke bhagwan hai hum.. abi badla lene aaye:)

Positive approach

This is what I call a Positive Approach!!