Wednesday, January 11, 2012

Comparing Java enum items...tiny bugs with a busines side effect

I continue to browse through numerous classes and packages, using the report from Sonar and the issues indicated by PMD and Findbugs. Most of the issues fixed are of trivial to medium severity (IMHO). It can be a bit boring task, there are moments though, where something small and trivial may lead to  well hidden business bug.

A method, a check somewhere that once fixed, might alter the business behavior of a component or a business rule.At the end of the day you get a small satisfaction of contributing a lot of small fixes towards a greater cause - feels nice - especially when the code base is really huge. Lots of KLOCs.

Many thanks to Findbugs for this - what a lovely library that is!

The problem: Sometimes comparing enum items using  equals() method may lead to a bug. 

There are lots of debates around comparing enum items and a really great post in StackOverflow. There are people who tend to compare enums using the '==' operator and others using the equals(). Both of them work (enum is a special thing anyway. I think I will go with the '==' use, since it might prevent you from accidentally ending with today's bug. I will provide very similar example derived from this answer (credits).
  
 enum AComplexNameForAnEnum { ITEM_ONE, ITEM_TWO };
 enum AComplexNameForSimilarEnum { ITEM_ONE, ITEM_TWO };

 //compiles fine - but the accident has happened
 if(AComplexNameForAnEnum.ITEM_ONE
        .equals(AComplexNameForSimilarEnum.ITEM_ONE)); 

 //wont compile at all
 if(AComplexNameForAnEnum.ITEM_ONE==AComplexNameForSimilarEnum.ITEM_ONE) 


The example is the same with the one provided in the threads answer, my point here about the accident about to happen is that 'we' (the developers) sometimes have to fight with huge codebases and projects where dozens of Enumerations and subtypes are declared.
Sometimes all these constructs come with similar names or of very similar business meaning. Using the equals() method, the compiler wont notice anything at all - technically it would have compiled a useless if statement but business wise we would have a business check resulting to a false result. Switching to the '==' operator the compiler would save us, to have a better loook on what we are trying to compare and change the subtype or use the appropriate construct. 

Sometimes complex naming can lead to such small errors. I am sort of a supporter of complex naming but I have to agree noticed that it can be error prone at the same level as a hack-named (short named) construct. By the way the findbugs warning was this (EC: Call to equals() comparing different types (EC_UNRELATED_TYPES).

Small and nice things, while browsing hude codebases. I am really enjoying this review thing since I can do a small research on each topic and maybe find all the related causes of a bug, rather than just a technical coding error.  

No comments:

Post a Comment