Best Practices: compareTo consistent with equals
September 2nd, 2007 |
What is wrong with this class and why? I’ll tell you beforehand there are two things I am looking for and they are both in the compareTo function. Yes, this came from the Palantir code base and caused me some issues. It has been modified slightly for illustrative purposes.
class GenericDataSource extends DataSource implements Comparable<dataSource> {
public int compareTo(DataSource o) {
if(o != null){
return getName().compareTo(o.getName());
}
return 1;
}
@Override
public boolean equals(Object o) {
if(o == null) return false;
if(o instanceof DataSource)
return this.getLocator().equals(((DataSource)d).getLocator());
return false;
}
}
According to the documentation for Comparable:
It is strongly recommended, but not strictly required that
(x.compareTo(y)==0) == (x.equals(y)). Generally speaking, any class that implements the Comparable interface and violates this condition should clearly indicate this fact. The recommended language is “Note: this class has a natural ordering that is inconsistent with equals.”
It is clear that this uses two totally different schemes for equals and compareTo. One uses the locator field and the other uses the name field. This means that this class has an inconsistent compareTo.
How would failing to have my natural ordering be consistent effect me? You would expect the following code to be true for collections that don’t contain null, but this is not the case for classes where the natural ordering is not preserved.
new HashSet<Type>(collection).size() == new TreeSet<Type>(collection).size()
Another issue with this compareTo function has to do with its handling of null. Since the compareTo function is reflexive even for null, the javadoc states that null must be handled by an NPE.
In the foregoing description, the notation
sgn(expression)designates the mathematicalsignumfunction, which is defined to return one of -1, 0, or 1 according to whether the value of expression is negative, zero or positive. The implementor must ensuresgn(x.compareTo(y)) == -sgn(y.compareTo(x))for allxandy. (This implies thatx.compareTo(y)must throw an exception if and only ify.compareTo(x)throws an exception.)
In short: in general, make sure that equals and compareTo are implemented in a coherent and consistent manner and it will save you some subtle and annoying headaches.






Don’t forget that the same advice applies to hashCode as well.
September 4th, 2007 at 12:24 pm
Nice tip. I have three little remarks:
Lines 10-11: “instanceof” returns false if the object “o” is null, so the first line is useless.
Line 12: the variable “d” should be “o”
Line 10: you can optimize (a bit) the method by adding this code
“if (this == o) return true;” on the first line. It will return true if you check an object against itself.
September 12th, 2007 at 12:05 am