Archive for category sonar

Code review checklist

A small checklist to conduct code reviews, is the change

  • Readable and following standards ?
  • Minimal and working solution ?
  • Better than before ?
  • Production ready ?
  • Checkout the details here :


    Leave a comment

    Through the eyes of sonar: recap.

    Sonar is an opensource platform to manage code quality. Sonar is combining various java quality tools and collect these possible defects as violations.

    Recap of all sonar rules articles

    Naming and comments
    Immutable Objects
    Exception handling
    Equals and hashCode
    Architural constraints
    Rules customization, alert,…

    I hope this will make your java code better

    Leave a comment

    Through the eyes of sonar : equals & hashCode.

    Equals and hashCode



    Indicates whether some other object is “equal to” this one.

    The equals method implements an equivalence relation on non-null object references:

    — It is reflexive: for any non-null reference value x, x.equals(x) should return true.
    — It is symmetric: for any non-null reference values x and y, x.equals(y) should return true if and only if y.equals(x) returns true.
    — It is transitive: for any non-null reference values x, y, and z, if x.equals(y) returns true and y.equals(z) returns true, then x.equals(z) should return true.
    — It is consistent: for any non-null reference values x and y, multiple invocations of x.equals(y) consistently return true or consistently return false, provided no information used in equals comparisons on the objects is modified.
    — For any non-null reference value x, x.equals(null) should return false.

    The equals method for class Object implements the most discriminating possible equivalence relation on objects; that is, for any non-null reference values x and y, this method returns true if and only if x and y refer to the same object (x == y has the value true).

    Note that it is generally necessary to override the hashCode method whenever this method is overridden, so as to maintain the general contract for the hashCode method, which states that equal objects must have equal hash codes.


    Returns a hash code value for the object. This method is supported for the benefit of hashtables such as those provided by java.util.HashMap or java.util.HashSet.

    The general contract of hashCode is:

    — Whenever it is invoked on the same object more than once during an execution of a Java application, the hashCode method must consistently return the same integer, provided no information used in equals comparisons on the object is modified. This integer need not remain consistent from one execution of an application to another execution of the same application.
    — If two objects are equal according to the equals(Object) method, then calling the hashCode method on each of the two objects must produce the same integer result.
    — It is not required that if two objects are unequal according to the equals(java.lang.Object) method, then calling the hashCode method on each of the two objects must produce distinct integer results. However, the programmer should be aware that producing distinct integer results for unequal objects may improve the performance of hashtables.

    As much as is reasonably practical, the hashCode method defined by class Object does return distinct integers for distinct objects. (This is typically implemented by converting the internal address of the object into an integer, but this implementation technique is not required by the JavaTM programming language.)


    Use the commons lang classes to help your code to be “nullsafe” and easier to implement :


    If you are using hibernate :
    — use the getter to avoid “proxy issues”
    — avoid using the id as equality (which isn’t always filled)
    — read the faq

    If you implement them… unit test your equals/hashCode
    — There’s a EqualsHashCodeTestCase that you can extends in the junit-addons library
    — note that there are also constraint for serialisation/clone
    — don’t call hashCode() but toHashCode() on hashCodeBuilder 😉
    — avoid reflection based equals/hashCode.

    Unit testing

    the EqualsHashCodeTestCase is extended in order to test a class’s functional compliance with the equals and hashCode contract.
    Override my createInstance and createNotEqualInstance methods to provide me with objects to test against. Both methods should return objects that are of the same class.
    will test :
    — whether equals holds up against a new Object (should always be false).
    — whether equals holds up against null.
    — whether equals holds up against objects that should not compare equal.
    — whether equals is consistent.
    — whether equals is reflexive.
    — whether equals is symmetric and transitive.
    — the hashCode contract.
    — the consistency of hashCode Across Invocations .

    Unit Tests and Serialization and serialization

    If you adopt a unit-testing methodology, then any serializable class should pass the following three tests:
    — If it implements readObject(), it should implement writeObject(), and vice-versa.
    — It is equal (using the equals() method) to a serialized copy of itself.
    — It has the same hashcode as a serialized copy of itself.

    Similar constraints hold for classes that implement the Externalizable interface.

    Sonar rules

    Warn – eclipse generated equals check class

    	public boolean equals(Object obj) {
    		if (this == obj)
    			return true;
    		if (obj == null)
    			return false;
    		if (getClass() != obj.getClass()) // BAD IDEA IF USING HIBERNATE
    			return false;

    Prefer instanceof check, because the “obj” may be a HibernateProxy dynamically extending MyObject

    	public boolean equals(Object obj) {
    		if (this == obj){
    			return true;
    		if (obj == null) {
    			return false;
    		if (!(obj instanceof MyObject)) {
    			return false;

    For the fields comparisons… prefer the EqualsBuilder


    Bad practice – Class defines equals() and uses Object.hashCode()

    This rule checks that classes that override equals() also need to override hashCode(). If you implement equals… you should implement hashCode accordingly.

    Bad practice – equals() method does not check for null argument

    As stated in the theory… “whether equals holds up against null”.
    A simple check with instanceof… could do the trick.

     public boolean equals(final Object object) {
       if (object.getClass().equals(this.getClass())) {
     	DecisionDetail decisionDetail = (DecisionDetail) object;
        	if (getId() == null || decisionDetail.getId() == null) {
        		return object == this;	

    See openpojo to test your model against such issue.

    Correctness – equals method always returns false / Correctness – equals method always returns true

    Having an equals method like this isn’t a good idea and doesn’t follow the contract on these methods

     public boolean equals(Object object) {		
       return true; // or false			

    If you want a good default implementation

     public boolean equals(Object object) {		
       return this==object;

    Performance – hashCode return a constant

    Returning a constant for the hashCode isn’t good for performance, as the hash is use to “compartiment/bucketing” data in structure like HashSet/hashMap.
    Returning a constant will put all the element in the same bucket transforming a lookup of O(1) to O(n) function.

    If you don’t have a clean equals criteria fallback to default implementation :

      public int hashCode() {
     	return System.identityHashCode(this);
      public boolean equals(Object object) {		
       return this==object;

    Correctness – equals() used to compare array and nonarray

    Comparing apples and oranges, an array of X won’t be equals to an object instance X



      public class PersonTypeCode extends Code {
          public static final PersonTypeCode[] ALL = { PROSPECT, MEMBER, PROSPECT_OR_MEMBER, PROSPECT_FAMILY_MEMBER };
          public static final PersonTypeCode PROSPECT = new PersonTypeCode("PROSPECT");

    Equals vs == :Literal Equality

    Literal Strings should be compared using equals(), not ‘!=’.

     String text = ...
     if (text != "") { //BAD

    Ideally… you should use StringUtils to compare in a null safeway.
    Note that you have StringUtils.isEmpty(...) or StringUtils.isBlank(...)

     String text = ...
     if (!StringUtils.isEmmpty(text)) {

    Class defines equal(Object); should it be equals(Object)?

    Surely a typo. If it’s not a typo choose a better name.

    Class defines hashcode(); should it be hashCode()?

    Surely a typo. If it’s not a typo choose a better name.

    Class defines tostring(); should it be toString()?

    Surely a typo. If it’s not a typo choose a better name.

    Dodgy – Potentially dangerous use of non-short-circuit logic

     return equalsRoot(other) & new EqualsBuilder().append(this.sourceId, other.getSourceId()).isEquals();

    This code seems to be using non-short-circuit logic (e.g., & or |) rather than short-circuit logic (&& or ||). In addition, it seem possible that, depending on the value of the left hand side, you might not want to evaluate the right hand side (because it would have side effects (hibernate initialization), could cause an exception (NullPointerException) or could be expensive (hibernate initialization).

     String s = null;   
     if (s == null || s.trim().length() == 0) {  
         // String s is really empty. Do some stuff.  
     if (s != null && s.trim().length() != 0) {  
        // String s is not empty. Do some stuff.  

    Both examples would throw a NullPointerException when s is null and you use | and & respectively. (Prefer StringUtils for this kind of check)

    Non-short-circuit logic causes both sides of the expression to be evaluated even when the result can be inferred from knowing the left-hand side. This can be less efficient and can result in errors if the left-hand side guards cases when evaluating the right-hand side can generate an error.

    , , ,

    Leave a comment

    Through the eyes of sonar : Collections.

    Collections usage

    After Exception handling, Immutable objects, Complexity, Naming and comments, let’s see what sonar can teach you about Collections.

    Use Arrays As List

    Use asList instead of tight loops

     final SWTBotTreeItem[] errorItemsFromNode =...
     for (int j = 0; j < errorItemsFromNode.length; j++) {		

    Can be rewritten as :

     errorItems = Arrays.asList(errorItemsFromNode)

    Note that the returned list is a non modifiable list. So if another part of the code tries to modify it will receive a UnsupportedOperationException

     java.util.List errorItems = new java.util.ArraysList(Arrays.asList(errorItemsFromNode));

    Performance – Inefficient use of keySet iterator instead of entrySet iterator

    The following code requires to do lookup (files.get(…)) and so evaluating hashCode on the key and while iterating on a Map and the doing a lookup the map implementation

     Map<String, Map<DateTime, File>> files = ...
      for (Iterator<String> iterator = files.keySet().iterator(); iterator.hasNext();) {
          final String serverName =;			
          final Map<DateTime, File> serverStats = files.get(serverName);

    You can prevent this, let’s use the entrySet() that will give you the key and associated value in a Map.Entry

    Map<String, Map<DateTime, File>> files = ...
      for (Map.Entry<String, List<MailServerStats>> entry : files.entrySet()) {
          final String serverName = entry.getKey();			
          final Map<DateTime, File> serverStats = entry.getValue();

    Loose coupling

    public class AST
           private HashMap<String, Object> properties = new HashMap<String, Object>();
           public HashMap<String, Object> getProperties() { 
                  return properties

    Please use collection interfaces (Map, Set, List) instead of concrete implementation (HashMap, HashSet, ArrayList, LinkedList,…) :

     public class AST
           private Map<String, Object> properties = new HashMap<String, Object>();
           public Map<String, Object> getProperties() { 
                  return properties

    This would allow you to easily switch implementation, in this case may be you want the map to be sorted.
    You just need to change the new HashMap… by a new TreeMap.

    Replace Vector With List

    Consider replacing this Vector with the newer java.util.List
    Vector synchronizes on each individual operation. That’s almost never what you want to do.
    Note that Vector is synchronized… and ArrayList/LinkedList are not.
    So if the synchronized nature is required in your code base
    you should use Collections.synchronized

     List list = Collections.synchronizedList(new ArrayList());

    Replace Hashtable With Map

    Consider replacing this Hashtable with the newer java.util.Map
    Hashtable synchronizes on each individual operation. That’s almost never what you want to do.

    note that Hashtable is synchronized… and HashMap/TreeMap/… are not.
    So if the synchronized nature is required in your code base
    you should use Collections.synchronized

     Map list = Collections.synchronizedMap(new HashMap());

    Class Cast Exception With To Array

    This usage of the Collection.toArray() method will throw a ClassCastException.

      return (String[]) values.toArray();

    the signature of the method : Object[] toArray()
    the ‘java.lang.ClassCastException: [Ljava.lang.Object; incompatible with [Ljava.lang.String;’
    but you have the other method T[] toArray(T[] a)

      return (String[]) values.toArray(new String[values.size()]);

    Note that if you use generics you can drop the cast :

      List<String> values = new ArrayList<String>();
     return values.toArray(new String[values.size()]);

    Correctness – Impossible downcast of toArray() result

    Same explanation as previous rules.

    	private static Map<String, String> linksreasonidtogroupid = new HashMap<String, String>();
    	public void testSample() throws Exception {
    	public static String[] getReasonsid() throws Exception {
    		Collection<String> values = linksreasonidtogroupid.values();
    		return (String[]) values.toArray();

    You endup with :

    java.lang.ClassCastException: [Ljava.lang.Object;
    	at CastTest.getReasonsid(
    	at CastTest.testSample(

    Correctness – Invocation of toString on an array

    Suppose you want to logs the variables in the following program :

    String[] vars = { "1", "2" };

    the output produced isn’t really helpfull


    instead use conditionnal logging and org.apache.commons.lang.ArrayUtils.toString (nullsafe and performant)

    String[] vars = { "1", "2" };
    if (LOG.isDebugEnabled()) {
    [1, 2]


    Through the eyes of sonar : Naming & Comments.

    Fourth “Through the eyes of sonar” article ( 1, 2, 3 ), let’s review various naming & comments rules.


    English: Illustration of all caps .vs. mixed case.

    English: Illustration of all caps .vs. mixed case. (Photo credit: Wikipedia)

    The excellent work of Cagdas Basaraner provide us these guidelines :

    • Use short enough and long enough variable names in each scope of code. Generally length may be
      • 1 char for loop counters,
      • 1 word for condition/loop variables,
      • 1-2 words for methods,
      • 2-3 words for classes,
      • 3-4 words for globals.
    • Use specific names for variables, for example "value", "equals", "data", … are not valid names for any case.
    • Use meaningful names for variables. Variable name must define the exact explanation of its content.
    • Don’t start variables with o_, obj_, m_, etc. A variable does not need tags which states it is a variable.
    • Obey company naming standards and write variable names consistently in application: e.g. txtUserName, lblUserName, cmbSchoolType, … Otherwise readability will reduce and find/replace tools will be unusable.
    • Obey programming language standards and don’t use lowercase/uppercase characters inconsistently: e.g. userName, UserName, USER_NAME, m_userName, username,
      • use Camel Case (aka Upper Camel Case) for classes: VelocityResponseWriter
      • use Lower Case for packages:
      • use Mixed Case (aka Lower Camel Case) for variables: studentName
      • use Upper Case for constants : MAX_PARAMETER_COUNT = 100
      • use Camel Case for enum class names and Upper Case for enum values.
      • don’t use ‘_’ anywhere except constants and enum values (which are constants).
    • Don’t reuse same variable name in the same class in different contexts: e.g. in method, constructor, class. So you can provide more simplicity for understandability and maintainability.
    • Don’t use same variable for different purposes in a method, conditional etc. Create a new and different named variable instead. This is also important for maintainability and readability.
    • Don’t use non-ASCII chars in variable names. Those may run on your platform but may not on others.
    • Don’t use too long variable names (e.g. 50 chars). Long names will bring ugly and hard-to-read code, also may not run on some compilers because of character limit.
    • Decide and use one natural language for naming, e.g. using mixed English and German names will be inconsistent and unreadable.
    • Use meaningful names for methods. The name must specify the exact action (ideally behavior) of the method and for most cases must start with a verb. (e.g. createPasswordHash)
    • Obey company naming standards and write method names consistently in application: e.g. getTxtUserName(), getLblUserName(), isStudentApproved(), … Otherwise readability will reduce and find/replace tools will be unusable.
    • Obey programming language standards and don’t use lowercase/uppercase characters inconsistently: e.g. getUserName, GetUserName, getusername, …
      • use Mixed Case for method names: getStudentSchoolType
      • use Mixed Case for method parameters: setSchoolName(String schoolName)
    • Use meaningful names for method parameters, so it can document itself in case of no documentation.

    Sonar Naming Ruleset

    Package Name

    A package is lowercase and avoid strange character

    Name ‘com.mestach.myBeans‘ must match pattern ‘^[a-z]+(\.[a-zA-Z_][a-zA-Z0-9_]*)*$‘.

    package com.mestach.myBeans; //bad
    package com.mestach.model;   //good

    Either you repackage to com.mestach.mybeans or com.mestach.model
    Note that windows isn’t case sensitive but linux is. So if you are using an old versioning system like CVS,
    Renaming once committed to a linux from windows may be harder (packages end up as folder : com/mestach/mybeans vs com/mestach/myBeans)

    Member Name

    Member Name : Name ‘FIELD_FIRSTNAME‘ must match pattern ‘^[a-z][a-zA-Z0-9]*$‘.

     protected String FIELD_FIRSTNAME = "fieldfirstname";

    — it’s a constant not well defined → add static final
    — it’s an attribute of the class and so it’s preferable to use the lowercase version : fieldFirstName

    Local Variable Name and Parameter Name

    Local Variable Name : Name ‘fromDate_db2TimeStamp‘ must match pattern ‘^[a-z][a-zA-Z0-9]*$

    String fromDate_db2TimeStamp = DATEFORMAT_db2TimeStamp.format(fromDate);

    Avoid underscore in variable name, java naming convention is more camelCase.

    String fromDateDb2TimeStamp = DATEFORMAT_db2TimeStamp.format(fromDate);

    Parameter and variable should starts by a lowercase :

    public void setMember(Member Member) // BAD
     public void setMember(Member member) //GOOD

    A more complex sample involving spring injection :

           private MyService module_MyService;
          public void setModule_MyService(MyService module_MyService){
                   this.module_MyService = module_MyService;


            private MyService myService;
            public void setMyService(MyService myService){

    Static Variable Name

    Sometime… it’s just a final which is missing :

    private static Logger LOG = Logger.getLogger(BatchLogger.class);   // BAD
    private static final Logger LOG = Logger.getLogger(BatchLogger.class); //GOOD

    Often in the code base you have (open) enum like this classes

    public class ObjectType {
       public static String SRVTYPE = "SERVER";
       public static String PRINTERTYPE = "PRINTER"
       public static ObjectType SERVER = new ObjectType(SRVTYPE );
       public static ObjectType PRINTER= new ObjectType(PRINTERTYPE );
       private String code;
       private ObjectType(String c) {
    	code = c;
       public String getCode() {
    	return code;

    Can be fixed by :

     public class ObjectType {
       private static final String SRVTYPE = "SERVER";
       private static final String PRINTERTYPE = "PRINTER"
       public static final ObjectType SERVER = new ObjectType(SRVTYPE );
       public static final ObjectType PRINTER= new ObjectType(PRINTERTYPE );

    — Don’t publish you SERVER string “constant”, make them private
    — The api using this ObjectType.SRVTYPE can be fixed by ObjectType.SERVER.getCode()

    Malicious code vulnerability – Field isn’t final but should be

    — Make your constant final ! It will prevent modifying the constant inadvertently

    ObjectType.SERVER =ObjectType.PRINTER

    Method Name

    Starts with lower case and use camelCase

      public String ProductReasonForm(Model model) {


      public String productReasonForm(Model model) {

    “Name methods after what they are intended to do.” (rather than how they do it) Kent Beck

    Bad practice – Class names shouldn’t shadow simple name of superclass

    This would help other developper to import the “correct” implementation.

      public class DwrController extends org.directwebremoting.spring.DwrController

    Just rename the class to avoid ambiguity.

      public class CustomDwrController extends org.directwebremoting.spring.DwrController

    Bad practice – Confusing method names

    Bad practice – Confusing method names : Confusing to have methods com.mestach.Person.getDeclarationWithincome() and com.mestach.Person.getDeclarationWithIncome()

    Did you notice the “case changes in method name “: Withincome vs WithIncome

    — either you wanted to override the method… and you mispelled it.
    — either you should perhaps rename it with a less confusing name.

    Use positive conditionals

    Use positive conditionals. Readability of positive conditionals are better than negative ones.

    Negated boolean variable names must be avoided.

     boolean isNotFound; // BAD
     boolean isNoError // BAD

    Positive !

     boolean isError; // GOOD
     boolean isFound; // GOOD

    The problem arise when the logical not operator is used and double negative arises.
    It is not immediately apparent what !isNotError means.

    Avoid boolean flags in method signature

    Don’t use method alternatives with boolean flag variables (public void someMethod(bool flag)). Write more than one method for each flag condition.

    Eg :

     Person loadPerson(Long id, boolean withContactInfos)


     /** don't the load the ContactInfos by default */
     Person loadPerson(Long id )
     Person loadPersonWithContactInfos(Long id )

    Or introduce an initializationMode

     enum PersonInitializationMode {
        Person loadPerson(Long id ,PersonInitializationMode...  initMode)

    More samples from Alex Ruiz’s boolean arguments can be evil

    In summary, let’s not use boolean parameters. They don’t communicate their purpose. Instead, we can:
    — in the case of methods, split them in two
    — in the case of constructors, use static factory methods
    — in the case of “three-state” booleans, use Java enums

    Use understandable and long names for variables.

    — Loop variable names can be i, j, k, index etc.,
    — local variable names must be longer than loop variables, parameter names must be longer than local variables and static variable names must be longer than parameters; proportional with scope size


     int currentLineNumber;
    double amount;


     int cln;
     double a;

    non-Static nested inner classes

    If possible make them static :
    — They are more loosely coupled to the parent class. In general, the weaker the coupling, the better.
    — They do not hold a pointer to the parent instance. This means:
    — They can exist without a parent instance.
    — They have a slightly smaller memory footprint.
    — They don’t prevent the parent from being GCed.
    And ideally move them outside, this will make the code more readable.

    A typical example where it causes issues : inner class that extends TimerTask gets registered with a Timer, and then the timer’s thread keeps both the TimerTask and the parent instance alive.

    Avoid static methods

    Think twice before defining a method as static and be sure if you really need to. Static methods are harder to manage :
    — test (state, singleton) and
    — evolve (methods overriding, use of beans, alternate implementation).


    Prefer a good naming over comments

    For example avoid abbreviation

      /** compute the average */
     double compAvg(double a ,double b)

    The name of the method explain it’s purpose !

    double computeAverage(double a ,double b)

    A comment is often never maintained as the code is refactored.
    Leaving the maintainer with comments that don’t match the reality anymore.

    Bad comment is worse than no comment.

    Don’t comment to delete code

    Don’t use comment lines to delete code, just delete. Version controlling system will help you if deleted code is required.

    Sonar can help you identifying this. In the comments widget you have the commented LOCs.

     70,143 lines 
     74.6% docu. API
     4,604 undocu. API
    1,770 commented LOCs
    Enhanced by Zemanta


    Through the eyes of sonar : Complexity


    I’m continuing the “Through the eyes of sonar” series ( 1 & 2 ) with various code complexity issues: First a small recap of guidelines/rules and then some practical examples.

    KISS Keep it simple & stupid

    1. Easy to read : Simple code doesn’t need additional documentation, or it only needs minimal documentation to be understood
    2. Easy to use : Whoever is using your code will find intuitive to use your objects.
    3. Easy to change : Simplicity means that there is no duplicated logic. A change should match a single place in your code.
    4. Doesn’t use any non-necessary third party tool or technology. Through my experience I have observed how some developers just love to use technologies, tools and frameworks just for the sake of making the project more “cool”. Every time you use them, you are adding extra complexity, and extra complexity means that is more difficult to understand, to maintain and is more prone to failures.
    5. It looks simple. If it doesn’t look simple, it is not simple! You know your code is simple if when you finish you are astonished at how simple the final solution is and you ask yourself how it was possible that it took you so long.
    6. Lean : It only does what is necessary, and nothing else. I think most seasoned developers would agree that trying to anticipate the problems your code will have to face in the future is impossible.
    7. Is straightforward. It doesn’t have unnecessary indirections; the most important operations only require a straight method call.

    Some basic rules

    Lets see some of the basic rules to write maintainable code :

    1. Keep methods short(one screen limit)
    2. Never ever ever reuse a variable for different purpose
    3. Use self-descriptive variable and method names
    4. Define variables as close as possible to the place of their usage
    5. No magic numbers
    6. Be friend with your language
    7. Don’t fight the convention
    8. Watch out for premature optimization
    9. Always refactor the code after you test it
    10. Don’t get sucked into over-engineering
    11. Learn new things by prototyping

    All these rules are explained with concrete examples here

    Cyclomatic complexity

    Tools like checkstyle will check the cyclomatic complexity of methods against a specified limit.

    The complexity is measured by the number of if, while, do, for, ?:, catch, switch, case statements, and operators && and || (plus one) in the body of a constructor, method, static initializer, or instance initializer. It is a measure of the minimum number of possible paths through the source and therefore the number of required tests.

    Generally :

    • 1-4 is considered good,
    • 5-7 ok,
    • 8-10 consider re-factoring, and
    • 11+ re-factor now !

    How we can reduce it… by implementing simple refactoring. Some of them are built-in in eclipse IDE, sample !

    Simplify Boolean Return (Conditional logic can be removed)

    Sometime we write unnecessary conditional logic :

    public boolean occupiesPosition(int x) {
       if (x >= position && x <= position + data.length())
          return true;
          return false;

    Can be rewritten :

    public boolean occupiesPosition(int x) {
      return (x >= position && x <= position + data.length())

    Note that some developer still like to introduce a boolean temporary variable to ease the debugging or logging, may be for those developer you can disable UnnecessaryLocalBeforeReturn rule.

    Simplify Conditional

    Sometime our paranoia of NullPointerException leads us to write unnecessary code. In this example there’s no need to check for null before an instanceof.

    public static Object[] getSelectedElements(final ISelection sel) {
       if (sel == null || !(sel instanceof IStructuredSelection))
         return null;
       IStructuredSelection strucSel = (IStructuredSelection) sel;

    Extract boolean expression in a method

    for (Iterator it = this.timeLines.values().iterator(); it.hasNext(); ) {
      final ITimeLine timeLine = (ITimeLine);
      final long start = timeLine.getStartTime().getMillis();
      final long end = timeLine.getEndTime().getMillis();
      if ( (start <= startTime && end >= startTime) ||
         (start >= startTime && end <= endTime) ||
         (start <= endTime && end >= endTime) ) {

    If you put the logic in a contains method, you end up with 2 methods of lower complexity and easier to read !

    for (Iterator it = this.timeLines.values().iterator(); it.hasNext(); ) {
      final ITimeLine timeLine = (ITimeLine);
      if ( contains(timeLine)) {

    And with the extracted methods :

    private boolean contains(ITimeLine timeLine) {
      final long start = timeLine.getStartTime().getMillis();
      final long end = timeLine.getEndTime().getMillis();
      return (start <= startTime && end >= startTime) || (start >= startTime && end <= endTime) || (start <= endTime && end >= endTime) )

    Replace if else if else by a map or a set

    For example sometimes you see a deep if else tree like this

    public boolean isManual() {
          if (MANUAL_CLERCK.equals(this))
             return true;
          else if (MANUAL_MEMBER.equals(this))
             return true;
          else if (MANUAL_INSURER.equals(this))
              return true;
          else if (MANUAL_DECL.equals(this))
    	     return true;
          return false;

    Adding a Set may simplify the if-else-if cascade to a simple contains

    private static final Set<IndicationOrigin> MANUAL_ORIGINS = new HashSet<IndicationOrigin>();
    static {
    public boolean isManual() {
       return MANUAL_ORIGINS.contains(this);

    Avoid ternary operator

    the following code throws a NullPointerException… despite the usage of the ternary operator to check for null value.

    DateMidnight startDate = null;
    LOGGER.debug(" ==> NPE " + startDate != null ? startDate.toString(Constants.DATE_FORMAT) : "null");

    In fact the priotiry in operators, will be evaluated like this (added parentheses) :

    LOGGER.debug((" ==> NPE " + startDate)!= null ? startDate.toString(Constants.DATE_FORMAT) : "null");

    So the boolean expression is always true (concat string) even if the startDate is null.
    To fix this you either drop the ternary operator or add the necessary braces to change the evaluation priority

    LOGGER.debug(" ==> correct log but full timestamp format " + startDate);
    LOGGER.debug(" ==> correct log " + (startDate != null ? startDate.toString(Constants.DATE_FORMAT) : "null"));

    The code maybe shorter, but you’ve won a bug, so avoid using the ternary operator.

    Dodgy – Method uses the same code for two branches

    This kind of constructs complexify the code :

    if (userLanguage == null || userLanguage.equals("fr")) {
    } else {

    Either you can remove the if else…


    Either you have a copy/paste error

    if (userLanguage == null || userLanguage.equals("fr")) {
     } else {


    Through the eyes of sonar: Immutable Objects.

    Sonar is an open platform to manage code quality. Sonar is combining various java quality tools and collect these possible defects as violations. Let’s see how sometimes we misuse immutable objects.

    Immutable Objects

    I’m continuing the “Through the eyes of sonar” series with all these immutable objects you use all the times.
    Immutable objects are simply objects whose state (the object’s data) cannot change after construction.

    Immutable objects greatly simplify your program, since they :
    — are simple to construct, test, and use
    — are automatically thread-safe and have no synchronization issues
    — do not need a copy constructor
    — do not need an implementation of clone
    — allow hashCode to use lazy initialization, and to cache its return value
    — do not need to be copied defensively when used as a field
    — make good Map keys and Set elements (these objects must not change state while in the collection)
    — have their class invariant established once upon construction, and it never needs to be checked again
    — always have “failure atomicity” (a term used by Joshua Bloch) : if an immutable object throws an exception, it’s never left in an undesirable or indeterminate state

    Examples of immutable objects
    — from the JDK include String, Integer, BigDecimal, Boolean, etc.
    — from joda time : DateMidgnight, DateTime,…

    More on this topic : ImmutableObjects


    Integer Instantiation

    Avoid instantiating Integer objects. Call Integer.valueOf() instead. This allows the class to give you back an existing instance if an object was already created for this number. Note this optimization that it’s more for small values of integer and depends on the jre implementation.

     return new Integer(rowCount);

    Prefer :

     return Integer.valueOf(rowCount);


    Why we don’t use Doubles for Financial Calculations

        package com.bloodredsun;
        public class DoubleOperation {
            public static void main(String[] args) {
                double t1 = 10.266d;
                double t2 = 10.0d;
                //Outputs 0.266
                double h1 = 100.266d;
                double h2 = 100.0d;
                //Outputs 0.26600000000000534

    Ouch! That is not what we want but it is the classic behavior of doubles. The inability to represent some decimals in the IEEE-754 format (as binary fractions) causes this. If we want correct precision the answer is to use BigDecimals but we have to remember to use Strings in the constructors or you end up with the same issues that you were trying to avoid.

        package com.bloodredsun;
        import java.math.BigDecimal;
        public class BigDecimalOperation {
                public static void main(String[] args) {
                BigDecimal t1 = new BigDecimal("10.266");
                BigDecimal t2 = new BigDecimal("10.0");
                //Outputs 0.266
                BigDecimal h1 = new BigDecimal("100.266");
                BigDecimal h2 = new BigDecimal("100.0");
                //Outputs 0.266

    Notice the usage of the string constructor ! You will read more about it in the the next paragraph about java.math.BigDecimal.


    Avoid Decimal Literals In Big Decimal Constructor

    As the BigDecimal class is immutable, all its methods modifying its value return a new BigDecimal object. The newly created object contains the value modified.

      BigDecimal dec = new BigDecimal("123.54");
      BigDecimal dec2 = new BigDecimal(123.54);
      System.out.println("dec value = " + dec);
      System.out.println("dec2 value = " + dec2);

    Result displayed in the console:

     dec value = 123.54
     dec2 value = 123.5400000000000062527760746888816356658935546875

    As Java doesn’t parse and represent these two numbers in the same way, the result is different.
    For this reason, when you use a BigDecimal object, create it with the constructor using a String in argument.

      BigDecimal dec = new BigDecimal(123.54);
      System.out.println("dec value = " + dec);
      BigDecimal dec2 = dec.setScale(2, BigDecimal.ROUND_HALF_UP);
      System.out.println("dec2 value = " + dec2);

    Result displayed in the console:

      dec value = 123.5400000000000062527760746888816356658935546875
      dec2 value = 123.54

    Useless Operation On Immutable

    As the setScale(newScale, roundingMode) method is used on an immutable object, it returns a new BigDecimal object with the modified value. If you don’t put the result of this method in another BigDecimal object, you will not able to work on this new value.
    Remember that the example above is valid for all methods involving the modification of a BigDecimal object’s value.

      BigDecimal dec = new BigDecimal("123.54");
      dec.add(new BigDecimal("1.0"));

    This will display :


    Fixing by reassigning the dec variable :

      BigDecimal dec = new BigDecimal("123.54");
      dec = dec.add(new BigDecimal("1.0"));
      dec = dec.add(BigDecimal.ONE);
      dec = dec.multiply(BigDecimal.TEN);

    This will display :


    All mathematical methods of the BigDecimal class ask you to give in argument a BigDecimal object containing the value to apply on the value of your BigDecimal object. To avoid the instantiation of a BigDecimal object just to add one or multiply your numbers by ten (as in the example), you can use the constant defined in the BigDecimal class. Use them to avoid creating multiple objects in memory.


    Use Joda Time

    Don’t use the mutable java built-in classes for date/time manipulation, prefer the use the immutable and fluent api of jodatime.

    Useless Operation On Immutable

    DateTime is also an immutable class. It suffers of the same advantages and inconveniences of the BigDecimal class.
    When you use the class, don’t forgot to provide an object to receive the result of your modification or they will be be never apply (cf. the following example) :

    public void testAddMonth() throws Exception {
      DateTime time = new DateTime();

    Result displayed in the console :


    No month added… damned.



    Avoid Instantiation

    The Boolean class is used instead of the primitive type in order to offer the possibility to have three values for a boolean (true, false and null). If you just need to use the classical true-false value, use the primitive type.

    Use primitives if you can, wrappers if you must

      new Boolean("true"); // bad practice : this will create always new instances prefer Boolean.valueOf(...)

    Prefer :


    If it is possible, use the constant defined in the Boolean class instead of creating a new object.

      if (x > 10){
      else {
      Boolean.valueOf(x > 10);

    The valueOf(boolean) method can receive in argument a condition which has as result true or false. As you can see in the example before, you can resolve the if-else code by using the valueOf(boolean) method.


    Useless Operation On Immutable

    String name = "David";
    name.replaceAll("D", "d");
    System.out.println("BAD   "+name);
    name= name.replaceAll("D", "d");
    System.out.println("GOOD  "+name);
    BAD David
    GOOD david

    Correctness – Method ignores return value

    I stumble upon this code

    String key = ...

    Either we forgot something to the index char returned or we can simply drop the call from the codebase.

    Avoid String To String

    Avoid calling toString on String

        StringBuffer buffer = new StringBuffer(4);
        if (socialState != null) {
    	buffer.append(" ");

    Simply :

        StringBuffer buffer = new StringBuffer(4);
        if (socialState != null) {
    	buffer.append(" ");

    Note also that in this code we should have used StringBuilder. Check this article :
    Using StringBuffer instead of StringBuilder

    Performance – Method invokes inefficient new String(String) constructor

    Avoid creating a new string :

       protected String getCode(final Code code) {
         return new String(code.getValue() + " " + getLabel(code));

    Simply :

     protected String getCode(final Code code) {
         return code.getValue() + " " + getLabel(code);

    Correctness – “.” used for regular expression

    The dot is a special character in regexp. Using String.split(regexp)… will not split

     String[] splitKeys = this.propertyDescriptor.getClassDescriptor().getName().split(".");

    A simpler and safer way is to use StringUtilsor the StringTokenizer where the dot is processed as a normal character

     String[] splitKeys = StringUtils.split(this.propertyDescriptor.getClassDescriptor().getName(),".")

    Inefficient String Buffering

    Note that string concatenation is often badly coded… some myth tells you that StringBuffer or StringBuilder are the way to go. But one thing that most developers don’t know… is that the compiler is smarter than you 😉


    will be compiled to :

     new StringBuilder("4564654").append(sdfds).append("1231321").toString()

    In some cases these optimizations are not enough, specially when looping on a list of records :

    strBuff.append("; " + contact.targetid);
    strBuff.append("; " +;
    strBuff.append("; " + task.dueDate);
    strBuff.append("; " + task.objectId);
    strBuff.append("; " + task.statusId);


    public static final String SEPERATOR = "; "

    NullSafety and trim/blank

    Instead of :

      if  (anAddress.getZipCode() == null || "".equals(anAddress.getZipCode().trim()))
         return false;

    Prefer the less error prone version using StringUtils :

      return StringUtils.isNotBlank(anAddress.getZipCode());