Through the eyes of sonar : equals & hashCode.

Equals and hashCode

Theory

equals

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.

hashCode

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.)

Recommandations

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

– EqualsBuilder
– HashCodeBuilder

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

see http://tomaszdziurko.pl/2009/05/eclipse-and-problem-with-auto-generated-equals/

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

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

If you want a good default implementation

//GOOD
 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 :

  @Override
  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

  !PersonTypeCode.ALL.equals(searchParameter.getPersonTypeCode())

Where

  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.

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++) {		
    errorItems.add(errorItemsFromNode[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 = iterator.next();			
      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>();
  values.add("1");
  values.add("2");
 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 {
		getReasonsid();
	}
	
	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(CastTest.java:18)
	at CastTest.testSample(CastTest.java:11)

Correctness - Invocation of toString on an array

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

String[] vars = { "1", "2" };
System.out.println(vars.toString());

the output produced isn't really helpfull

[Ljava.lang.String;@10b4199

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

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

[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.

Guidelines

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: com.company.project.ui
  • 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";

Either
– 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 :

        //BAD
       @Autowired
       private MyService module_MyService;
      public void setModule_MyService(MyService module_MyService){
               this.module_MyService = module_MyService;
      }

Prefer

        //GOOD
        @Autowired
        @Qualifier("module_MyService")
        private MyService myService;
        public void setMyService(MyService myService){
            this.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

 //BAD
  public String ProductReasonForm(Model model) {

Prefer

 
//GOOD
  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)

Prefer

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

Or introduce an initializationMode

 enum PersonInitializationMode {
     WITHCONTACTINFOS, WITHACCOUNTNUMBERS
  }

    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

Prefer

 int currentLineNumber;
double amount;

over

 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).

Comments

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.

 Comments
 28.6%
 70,143 lines 
 74.6% docu. API
 4,604 undocu. API
1,770 commented LOCs