> The best solution is to make collections take a Equalator
> interface (similar to a Comparator). All of this nonsense
> goes away if you do that. Unfortunately the repeated
> cries for this feature from developers has been ignored by
> Sun for years.
Yes, very true. Apache collections allows you to do that and I've added a feature request for google collections (which uses generics).

http://code.google.com/p/google-collections/issues/detail?id=188

http://commons.apache.org/collections/api-release/org/apache/commons/collections/map/AbstractHashedMap.html#isEqualKey(java.lang.Object,%20java.lang.Object)

In Pitfall #1, #2, and #3:

result = (this.getX() == that.getX() && this.getY() == that.getY());

I would prefer:

result = that.xEquals( getX() ) && that.yEquals( getY() );

Or even:

result = that.equals( getX(), getY() );

This:

* avoids duplicating code (writing this.getX() == that.getX() will likely occur more than once);
* allows changing of the underlying data type (as Java cannot override a method signature based on return type alone);
* allows subclasses to inject behavioural changes, such as logging (ideally without breaking LSP);
* promotes encapsulation: the object containing the data performs tasks upon it; and
* is shorter.

The equals method then reduces to:

@Override public boolean equals(Object other) {
    return other instanceof Point ? ((Point)other).equals( getX(), getY() ) : false;
}

For Pitfall #3, you could also implement an Observer - Observable pattern so that the collection can rehash its objects if any of them are changed. Using a separate method, such as equalsContents, however, is simpler.

For the section on the canEqual method, I would evoke the same encapsulation metaphor:

result = that.canEqual( this ) && that.colorEquals( getColor() ) && super.equals( that );

Note that the color instance variable, even though it was declared final, is best not touched directly (at least until a new revision of Java is released that is like Smalltalk in how it handles instance variables). It seems to me that direct manipulation of class-scope instance variables and null data have needlessly introduced more bugs in the various Java-based systems I have worked on than all other problems combined! (I am, of course, exaggerating.)

I still feel like there is some redundancy in the canEquals and equals methods. Specifically:

    @Override public boolean equals(Object other) {
        boolean result = false;
 
        if (other instanceof ColoredPoint) {

And:

    @Override public boolean canEqual(Object other) {
        return (other instanceof ColoredPoint);

I have not worked out the logic (reversing this and that might be incorrect), but perhaps the equals method can be reduced to:

    @Override public boolean equals(Object other) {
        boolean result = false;
        if( this.canEqual( other ) ) {
            ColoredPoint that = (ColoredPoint) other;
            result = that.colorEquals( getColor() ) && super.equals( that );
 
        }
        return result;
    }

Which would further reduce to:

    return this.canEqual( other ) ?
      ((ColoredPoint)other).colorEquals( getColor() ) && super.equals( that ) :
      false;

An exceptional article; thank you for writing it!

Angelika Langer and Klaus Kreft already did a good work about: http://www.angelikalanger.com/Articles/JavaSolutions/SecretsOfEquals/Equals-2.html

Tonyx

One problem is using a HashSet for containing objects of different but related types.

The other problem is that a derived class must not change the contract of the base class. Thus, it should not redefine "equals".

MHO is that ColoredPoint sucks as an example. In real, responsible programming, the new features should be added to Point until it becomes a fat class, or Design Patterns should be used to achieve the same through delegation, decoration, or proxies.

A collection of Point must respond to the Point contract, and nothing else. Period (point).

Apparently, a careful design is needed to avoid issues with equals() usage. However, I think in some cases the problem is just a lack of the flexibility which could have been provided by some kind of an Equalator class - as mentioned already in some previous posts.

Let's revisit the example used by Joshua Bloch to illustrate one of his concerns regarding the equals() usage. The issue was the following method of the Point class (which could have been provided within some unrelated utility class as well):

public static boolean onUnitCircle(Point p) {
return unitCircle.contains(p);
}

Here unitCircle is a HashSet<Point> collection of the appropriate Point objects.

Now if we have a ColorPoint class which is derived from Point but redefines its equals() method, it turns out onUnitCircle() can't be reused properly to process ColorPoint objects as well. There is no consistent implementation of ColorPoint.equals() which could accommodate such usage. Consequently, none of the implementations of equals() suggested earlier in this discussion thread can resolve this issue either.

In order to support a safe reuse of methods like onUnitCircle(), Joshua Bloch recommends avoiding the inheritance and having a Point object as a member of the ColorPoint class. Then asPoint() method could be used to pass such Point object to methods which require Point-based equals() behavior.

But I think even this approach is not quite good enough, and abandoning the inheritance relationship can significantly impact the ability to reuse the Point-based utilities.

For example, in the context of usage there might be some Point method calls which are expected to be overridden by its derived classes, like:

public static void process(Point p) {
if (onUnitCircle(p))
p.show();
}

Here p.show() is expected to behave as appropriate for each kind of Point object. But if we pass ColorPoint.asPoint() object, it won't represent properly that aspect of the ColorPoint behavior (the point might get displayed in some default color).

Further on, it would be quite inconvenient to reuse the utilities like the following ones, since new Point collections would have to be created and passed as input arguments:

public static void reflectOnX(List<? extends Point> a);
public static boolean checkForOverlap(List<? extends Point> a,List<? extends Point> b);

However, if it's feasible to modify the Point-related code, Point class could be redesigned as having a Position object exposed via get/setPosition() methods. Then reusable utility methods could be achieved by applying the Position-based equality as needed: using HashSet<Position> objects and passing Point.getPosition() to contains() method. Still, such Position class wouldn't be necessary if there was the ability to pass the appropriate Equalator instance to the HashSet<Point> constructor and thus support the required context-specific equality.

Aside from the issue I discussed in my previous post, I think that for "non-value-added" class hierarchies an implementation of equals() similar as suggested by Gregor Zeitlinger could be appropriate. However, that implementation would cause an infinite recursion when the objects belong to different branches of the same hierarchy. So here is a revised and slightly repackaged implementation:

public final boolean equals(Object o) { return equalsImpl(o,true); }

protected boolean equalsImpl(Object o,boolean isFirstPass)
{
    // reject if not an object from the base class hierarchy 
    if (!(o instanceof Point))
        return false;
   	
    Point p = (Point) o;

    // delegate if it's the first pass/call of this method 
    // and the object belongs to one of the subclasses
    if (isFirstPass && !(p.getClass().isInstance(this)))
        return p.equalsImpl(this,false);

    // reached for all objects from the base class hierarchy
    // which classes just inherit Point.equalsImpl() method 
    return (x == p.x && y == p.y);
}

Any further derived classes which don't add new values can simply rely on equals() implementation in the base Point class. Otherwise, a derived class could override equalsImpl() in a similar manner. However, in such cases issues like one raised by Joshua Bloch might occur - as I pointed out in my previous post.

Also, I've noticed there's somewhat similar implementation of equals() provided by James Ahlborn in response to the following article by Angelika Langer and Klaus Kreft:

http://www.angelikalanger.com/Articles/JavaSolutions/SecretsOfEquals/Equals.html

Note that in some cases that implementation might evaluate the same parameters twice.

Further on, as mentioned earlier, there is a follow-up article:

http://www.angelikalanger.com/Articles/JavaSolutions/SecretsOfEquals/Equals-2.html

In that article more complex implementation of equals() is proposed, which covers value-added classes as well. The equality is based on the comparison of common parts and the notion of default values for attributes which are not common. It's defined in such way that typical non-transitivity issues are avoided. For example, a Point object is equal to only one ColorPoint object - the one which has the same position and the "default" color.

While such approach provides a formally correct implementation of equals() which might be useful in some cases, it still can't address context-specific needs - like in the example I discussed in my previous post. In such cases the only consistent solution is to expose the relevant object parts and use their own equals() method.

Note that the article is pretty old, from 2002. The following year there was a little Artima article by the same authors:

http://www.artima.com/weblogs/viewpost.jsp?thread=6543

where they say:

"...Or take the inconsistency between comparison and quality: we can support several sorting orders via comparators, but we can only have one notion of equality via equals(). Why? ...

Hi Bill

Thanks for a very interesting article that I got a link to a few weeks back and has been bothering me ever since. A problem that is hard to pin down but has a simple solution when you get there, maybe. Perhaps too simple, what you think?

Start with some isA or similar comparisons, remove those that are not obligatory and you get left with almost nothing:

public class Point {
 
    private final int x;
    private final int y;
 
    // ................
 
    @Override public boolean equals(Object obj) {
        return (obj instanceof Point &&
                    ((Point)obj).fEquals(this));
    }
 
    protected boolean fEquals(Point obj) {
        return (this.x == obj.x && this.y == obj.y);
    }
 
    @Override public int hashCode() {
           // ................
}
 
public class ColoredPoint extends Point {
 
    private final Color color;
 
    // ................
 
    @Override public boolean equals(Object obj) {
        return (obj instanceof ColoredPoint &&
                    ((ColoredPoint)obj).fEquals(this));
    }
 
    protected boolean fEquals(ColoredPoint obj) {
        return (this.color.equals(obj.color) && super.fEquals(obj));
    }
 
    @Override protected boolean fEquals(Point obj) {
        return false;
    }
 
    @Override public int hashCode() {
           // ................
}

And so on down the inheritance chain to any depth you like. Surely someone else must have though of doing this before now and then rejected it for some reason? None of the how to override equals() postings I have seen suggest it but if you need an equals() that can be overridden or not as required with minimum additional coding and overhead it looks like an answer.


Took a while to get to this embarrassingly naive solution. There is a blow by blow account including an equals() for Equality of the Third Kind at

http://martinsjava.blogspot.com/2009/11/overriding-equalsobject-optimally.html

Running the your code (with the appropriate constructors, etc., added in)

        Point p1  = new Point(1, 1);
        Point p2  = new ColoredPoint(1, 1, Color.PINK);
        ColoredPoint cp1 = new ColoredPoint(1, 1, Color.PINK);
        
        System.out.println("p1.equals(p2) is " + (p1.equals(p2)));
        System.out.println("p2.equals(p1) is " + (p2.equals(p1)));
        System.out.println("p1.fEquals(p2) is " + (p1.fEquals(p2)));
        System.out.println("p2.fEquals(p1) is " + (p2.fEquals(p1)) + "\n");
        
        System.out.println("p1.equals(cp1) is " + (p1.equals(cp1)));
        System.out.println("cp1.equals(p1) is " + (cp1.equals(p1)));
        System.out.println("p1.fEquals(cp1) is " + (p1.fEquals(cp1)));
        System.out.println("cp1.fEquals(p1) is " + (cp1.fEquals(p1)) + "\n");
        
        System.out.println("p2.equals(cp1) is " + (p2.equals(cp1)));
        System.out.println("cp1.equals(p2) is " + (cp1.equals(p2)));
        System.out.println("p2.fEquals(cp1) is " + (p2.fEquals(cp1)));
        System.out.println("cp1.fEquals(p2) is " + (cp1.fEquals(p2)));

yields:

p1.equals(p2) is false
p2.equals(p1) is false
p1.fEquals(p2) is true
p2.fEquals(p1) is false

p1.equals(cp1) is false
cp1.equals(p1) is false
p1.fEquals(cp1) is true
cp1.fEquals(p1) is false

p2.equals(cp1) is true
cp1.equals(p2) is true
p2.fEquals(cp1) is false
cp1.fEquals(p2) is false

One immediate point (sic) to note is the asymmetric fEquals test which can return true or false when testing the same objects, depending on which object you ask.

Hi Vincent

The reason you are getting asymmetrical and strange results is because you are calling the protected fEquals() methods from main() - the fact that this can be done is down to Java weak same-package encapsulation, not my fault. These methods should not be called from outside of the class but cannot be private because they need to be overridden in subclasses - protected is the best that can be done.

In equals() it is only the combination of the instanceof check and calling the received object's version of fEquals() with the current object that makes the technique work. Please have a look at my blog link if you would like a detailed breakdown - you may like to skip the analysis of equals() requirements and go straight to 'Devising a solution'.

Your result p2.fEquals(cp1) is false is interesting. I had not considered what would happen if these methods were called from outside of the class against a reference to a different class of object. Fortunately inside of equals() there is no doubt about type and everything works ok as shown by the equals() results in your output.

A few comments:

1. Bloch's recommendations to include a reference equality short circuit seem to have been ignored here. The cleanest version would be

@Override
public boolean equals( Object other ) {
    Point that;
    return this == other || (
        canEqual( other ) && ( that = ( Point )other ).canEqual( this ) &&
        getX() == that.getX() && getY() == that.getY()
    );
}

2. The value add here is not so much that code duplication is avoided; it is that writing an equals() method that is more flexible than the one in Effective Java and still compliant is now 100% boilerplate and could conceivably be generated by an IDE. Follow the formula and you not only don't have to think about how to implement equals()/canEqual(), but you also get to extend concrete classes in non-state-related ways and preserve the contract. It is also more efficient than the blindlyEquals()/fEquals() solutions, although those formulations might have other benefits.

3. I have approached the boilerplate equality method using reflection, keeping a static cache mapping every class to the class which defines its equals (determined via reflection for cache misses). Then equals() always looks like

@Override
public boolean equals( Object other ) {
    C cOther;
    return this == other || (
        eqPrereq( this, other ) && // for direct subclasses of Object
//      super.equals( other ) && // otherwise
        nullSafeEquals( this.getProp1(), ( cOther = ( ( C )other ) ).getProp1() ) &&
        nullSafeEquals( this.getProp2(), cOther.getProp2() ) && ...
    );
}

where eqPrereq is

return oOther != null && getEqualsClass( oThis ).equals( getEqualsClass( oOther ) );

This way you don't need to implement anything that is not already in Object, and eqPrereq is implemented once (in a commons package).

Am i missing something? Wasn't .equals solved:

Point:

  public boolean equals (final Object obj) {
    if (this == obj) {
      return true;
    }
    if (obj == null) {
      return false;
    }
    if (this.getClass() != obj.getClass()) {
      return false;
    }
    final Point other = (Point) obj;
    if (this.x != other.x) {
      return false;
    }
    if (this.y != other.y) {
      return false;
    }
    return true;
  }

ColorPoint:

  public boolean equals (final Object obj) {
    if (this == obj) {
      return true;
    }
    if (!super.equals(obj)) {
      return false;
    }
    if (this.getClass() != obj.getClass()) {
      return false;
    }
    final ColoredPoint other = (ColoredPoint) obj;
    if (this.color == null) {
      if (other.color != null) {
        return false;
      }
    } else if (!this.color.equals(other.color)) {
      return false;
    }
    return true;
  }

And for the cases were you need symmetry (which breaks the .equals contract due to the impossibility of implementing symmetry and transitivity through a hierarchy), use the following to delegate to super classes:

...
    if (this.getClass() != obj.getClass()) {
      if (obj.getClass().isAssignableFrom(this.getClass())) {
        return obj.equals(this);
      }
      if (!this.getClass().isAssignableFrom(obj.getClass())) {
        return false;
      }
    }
...

Doesn't this accomplish the same as canEqual but without the need for an additional method?

@Override public boolean equals(Object other) {
boolean result = false;
-->> if (other instanceof ColoredPoint) {
ColoredPoint that = (ColoredPoint) other;
result = (that.canEqual(this) && this.color.equals(that.color) && super.equals(that));
}
return result;
}

-->> @Override public boolean canEqual(Object other) {
return (other instanceof ColoredPoint);
}


you checked twice ,why ?

Nice post, i liked the canEqual method name :). Here are my thoughts on it.

Keeping the canEquals method in hands of a sub class makes it design by discipline, not by mandate.

How about writing the canEquals method like this in the super class. I guess it may eliminate the need to subclasses overriding it.

@Override public boolean canEqual(Object other) {
        return (other instanceof this.getClass());
    }

However, the above implementation can eliminate any possibilities of subclasses making mistakes.

Sorry for posting to such an old thread..
From the article:

    @Override public boolean equals(Object other) {
        boolean result = false;
        if (other instanceof ColoredPoint) {
            ColoredPoint that = (ColoredPoint) other;
            result = (this.color.equals(that.color) && super.equals(that));
        }
        else if (other instanceof Point) {
            Point that = (Point) other;
            result = that.equals(this);
        }
        return result;
    }

Won't that recurse forever, or until there is a stack overflow error, if you use it with a different subclass of Point that does something similar?

Thanks very much for the article, I read it a while ago and have recently come back to it. Very informative.

However, I _think_ I may have come up with a slightly neater version of the canEqual() solution.

Assuming that:
(a) You don't override equals() unless you intend to restrict comparisons to a sub-branch of the hierarchy.
(b) When you do override equals(), you ALSO override canEqual() to check for the appropriate subclass.
it seems to me that you could write your equals() methods as follows:

For Point (or the top class in the hierarchy):

   public boolean canEqual(Object other) {
      return other instanceof Point;
   }
 
   @Override
   public boolean equals(Object other) {
      if (other == this)
         return true;
 
      if (!canEqual(other))
         return false;
 
      // Assuming canEqual() is overridden to check for a
      // subclass of this one, the following assignment
      // must work:
      Point that = (Point) other;
 
      // Check that that.canEqual(this) AND invariants match. 
      return that.canEqual(this) && x == that.x & y == that.y;
    }

For ColoredPoint (or a subclass in the hierarchy):

   @Override
   public boolean canEqual(Object other) {
      return other instanceof ColoredPoint;
   }
 
   @Override public boolean equals(Object other) {
      if (other == this)
         return true;
 
      if (!super.equals(other))
         return false;
 
      // We now know that 'other' must be a ColoredPoint
      // because the 2nd check in the top-level equals() 
      // method will have called OUR canEqual() method.
      // So, the following assignment must work:
      ColoredPoint that = (ColoredPoint) other;
 
      // We now only need to check invariants because 
      // super.equals(other) can't return true unless
      // other.canEqual(this).
      // So...
      return color.equals(that.color);

I haven't benchmarked it, but I suspect that performance will be slightly worse than the style posted in the article, but it it just seems more logical to me from a hierarchical standpoint:
1. It eliminates reflexivity quickly.
2. It makes sure that superclass values (and type) are checked FIRST.
3. It makes use of the polymorphic nature of canEqual().
I also suspect that it would take less massaging to work properly for more sophisticated hierarchical comparisons (but it's just a feeling; don't ask me for proof :-) ).

Comments (even "ít's crap") gratefully received - especially if someone's already sussed out the same thing.

Winston