Summary of Recommendations

When defining or refactoring equals an hashcode methods:

• most classes should not override equals. Objects which will go into a Hibernate Set need to override it, though.
• If using eclipse, use the "Generate hashcode and equals..." item from the Source menu as a starting point for these methods. (If not using eclipse, use this article as a guide)
• test the equals method using mifos TestUtils.verifyBasicEqualsContract method as a starting point and add aditional tasks as necessary to get good test coverage. org.mifos.framework.components.configuration.cache. TestKey is an example of a test case which uses the method. Be sure to include test sfor null field values.
• if equals is being implemented for a persistent class, replace the eclipse generated getClass check:

  if (obj == null
     return false;
  if (getClass() != obj.getClass())
     return false;
  

with an instance of check:

if(!(object instanceof equalsClass)) return false;

and use getter methods rather than direct access to get field values. This is to avoid problems with using lazily loaded proxies in Hibernate. Make the equals method final in this case.

Background

Many existing equals methods in Mifos have issues as flagged by findbugs. As we go about fixing these, it would be useful to try to come to a consensus on what a preferred style of equals method would look like and how to test it.

If you use Eclipse then the eclipse generated equals and hashcode methods are a good starting point for the equals and hashcode methods that we define (this feature can be found by going to the Source menu and selecting "Generate hashcode and equals..."). The equals method that is generated can seem a bit verbose, but it does all the necessary null checking and follows what is generally agreed upon as a correct approach to defining equals. (Note, if you are not using Eclipse, the first article listed below includes a step by step guide to implementing a good equals method).

The one point over which there is some debate is which of the following two idioms the equals methods should use (in this example assume a method: public boolean equals(Object object) for the class equalsClass):

if(!(object instanceof equalsClass)) return false;

or

if((object == null) || (object.getClass() != this.getClass())) return false;

There are use cases where each of these is appropriate, and use cases where each of these is inappropriate (for example, the first case could be appropriate if a class is extended to add new methods without adding new data fields. The second case is appropriate if a class is extended and new data fields are added-- see the references below for complete examples). So the bottom line is that it does take some thought as to what is appropriate for a given case.

The getClass() approach is arguably somewhat safer to use as the default and so the eclipse generated method would be reasonable to start with and modify if you know the case you are working with requires it. This does go against recommendations in some popular Java texts such as "Effective Java", but it is a widely used approach. Below are some articles that present arguments on both sides:

1. http://www.geocities.com/technofundo/tech/java/equalhash.html
2. http://www.artima.com/intv/bloch17.html
3. http://www.agiledeveloper.com/articles/equals062002.htm

Another question with regard to the equals operator is how it how to test it. It would be useful to have a standard pattern to be followed to cover testing the basic requirements of the equals method. The TestUtils class contains our equals/hashcode testing framework. There is a verifyBasicEqualsContract methods that attempts to encapsulate the basic testing to be done whenever an equals method is defined. There may well be more equals testing to be done for a give class than just calling verifyBasicEqualsContract, but if verifyBasicEqualsContract is included in the tests, then we know that the basics are covered.

Finally, how an equals method is defined for persistent Mifos classes is impacted by the use of Hibernate. There are a number of classes which use a hibernate generated id for the equals method (eg. COABO, CollSheetCustBO, CollSheetLnDetailsEntity, CollSheetSavingsDetailsEntity, MifosCurrency, and more). This can cause problems if these objects are used before they have been saved to the database since an id is generated only after saving for the Hibernate generator type "native" which is used in Mifos.

We need to be aware of these limits and how it affects our usage. Unfortunately there doesn't appear to be agreement on what the best way is to address this issue. It is noted here to raise awareness since it could cause incorrect behavior. An explanation of the issue can be found at http://www.hibernate.org/109.html (Note: if you read the commentary on this page you will see that this is not a simple issue and that various problems exist with different approaches depending on usage). There is more discussion at http://forum.hibernate.org/viewtopic.php?t=928172.

Another issue with Hibernate is the use of direct access to member fields by the equals method. If Hibernate objects are loaded using a proxy (as is the case when lazy loading is used), then an equals method needs to use getters to work correctly. This is not what Eclipse generates, so it means that the Eclipse generated method needs to be edited to use getters for objects persisted using Hibernate.