Should I be concerned about this compareTo/equals/hashCode implementation?

Question

I'm in the middle of QA'ing a bunch of code and have found several instances where the developer has a DTO which implements Comparable. This DTO has 7 or 8 fields in it. The compareTo method has been implemented on just one field:

private DateMidnight field1;  //from Joda date/time library

public int compareTo(SomeObject o) {
   if (o == null) {
      return -1;
   }
   return field1.compareTo(o.getField1());
}

Similarly the equals method is overridden and basically boils down to:

return field1.equals(o.getField1());

and finally the hashcode method implementation is:

return field1.hashCode;

field1 should never be null and will be unique across these objects (i.e. we shouldn't get two objects with the same field1).

So, the implementations are consistent which is good, but should I be concerned that only one field is used? Is this unusual? Is it likely to cause problems or confuse other developers? I'm thinking of the scenario where a list of these objects are passed around and another developer uses a Map or Set of somesort and gets unusual behaviour from these objects. Any thoughts appreciated. Thanks!

Answer 1

I suspect that this is a case of "first use wins" - someone needed to sort a collection of these objects or put them in a hash map, and they only cared about the date. The easiest way of implementing that was to override equals/hashCode and implement Comparable<T> in the way you've said.

For specialist sorting, a better approach would be to implement Comparator<T> in a different class... but Java doesn't have any equivalent class for equality testing, unfortunately. I consider it a major weakness in the Java collections, to be honest.

Assuming this really isn't "the one natural and obvious comparison", it certainly smells in terms of design... and should be very carefully document.

Answer 2

Strictly speaking, this violates the Comparable spec:

http://download.oracle.com/javase/6/docs/api/java/lang/Comparable.html

Note that null is not an instance of any class, and e.compareTo(null) should throw a NullPointerException even though e.equals(null) returns false.

Similarly, it looks like the equals method will throw NPE on equals(null) instead of returning false (unless of course you "boiled" out the null handling code).

Is it likely to cause problems or confuse other developers?

Possibly, possibly not. It really depends on how large your project is and how widespread/"reusable"/long-lived your object source code is expected to be used:

• Small/short-lived/limited use == probably not a problem.
• Large/long-lived/widespread use == counter-intuitive implementation may cause future problems

Answer 3

You shouldnt be concerned with it, if field1 is really unique. If it`s not, you may have problems. Anyway, my advise is to do some unit tests. They should show the truth.

Answer 4

I don't think you need to be concerned. The contract between the three methods is kept and it's consistent.

Whether it's correct from a business logic point of view is a different question.

If e.g. field1 maps to a primary key in the database it's perfectly valid. If field1 is the "firstname" of a person, I would be concerned