A Java Gotcha October 28, 2005 | 10:31 am

Java has a universal “equals” method, as well as a “Comparable” interface that defines “natural comparison”. I always had a distaste for that design decision, and that distaste has just been further validated.

So, I have a class which looks like this:

     public class TwoDeePoint implements Comparable {
          public int x = 0;
          public int y = 0;
 
          public boolean equals(final Object obj)  {
                    if(obj == null || !(obj instanceof TwoDeePoint)) {
                              return false;
                    } else {
                              TwoDeePoint them = (TwoDeePoint)obj;
                              return them.x == this.x && them.y == this.y;
                    }
          }
 
          public int compareTo(final Object o) {
                    if(o == null) {
                              throw new NullPointerException();
                    } else if(!(o instanceof TwoDeePoint)) {
                              throw new ClassCastException();
                    } else {
                              final TwoDeePoint them = (TwoDeePoint)o;
                              final int xCmp = new Integer(this.x).compareTo(new Integer(them.x));
                              final int yCmp = new Integer(this.y).compareTo(new Integer(them.y));
                              return xCmp != 0 ? xCmp : yCmp;
                    }
          }
     }

Even ignoring certain religious/performance fights (like whether the trinary operator is evil, why I declare so much crap “final” and whether the “x” member variable should be “X“, or even visible at all…), there is a major error to this code. Specifically, consider this class:

     public class ThreeDeePoint extends TwoDeePoint implements Comparable {
          public int z = 0;
 
          public boolean equals(final Object obj)  {
                    if(obj == null || !(obj instanceof ThreeDeePoint)) {
                              return false;
                    } else {
                              final ThreeDeePoint them = (ThreeDeePoint)obj;
                              return super.equals(obj) && them.z == this.z;
                    }
          }
 
          public int compareTo(final Object o) {
                    if(o == null) {
                              throw new NullPointerException();
                    } else if(!(o instanceof ThreeDeePoint)) {
                              throw new ClassCastException();
                    } else {
                              final ThreeDeePoint them = (ThreeDeePoint)o;
                              final int supCmp = super.compareTo(them);
                              final int zCmp = new Integer(this.z).compareTo(new Integer(them.z));
                              return supCmp != 0 ? supCmp : zCmp;
                    }
          }
     }

See the problem yet? Guess the result of the following snippet of code:

          final TwoDeePoint a = new TwoDeePoint();
          a.x = 1;
          a.y = 2;
          final ThreeDeePoint b = new ThreeDeePoint();
          b.x = a.x;
          b.y = a.y;
          c.z = 3;
          System.out.println(a.equals(b));
          System.out.println(b.equals(a));

The answer is that the snippet will print out:

          true
          false

And there’s the problem: the equality operator, which is designed into all classes and is so critical to the operation of so many standard collections, has a very common and glaring issue. Given this implementation, a Set of TwoDeePoint and ThreeDeePoint objects may or may not insert a new ThreeDeePoint if its x and y are the same as an existant TwoDeePoint, and the deciding issue which equals method the programmer decided to call. Since the equals method is reflexive by definition, it shouldn’t make a difference…but it does.

The kludge that all Java programmers need to burn permenently into their brain can be modelled after the following change to the TwoDeePoint class:

          public boolean equals(final Object obj)  {
                    if(obj == null || !(obj instanceof TwoDeePoint)) {
                              return false;
                    } else {
                              TwoDeePoint them = (TwoDeePoint)obj;
                              if(them.x == this.x && them.y == this.y) {
                                        if(them.getClass() == this.getClass()) {
                                                  return true;
                                        } else {
                                                  return them.equals(this);
                                        }
                              }
                    }
          }

At the point when we do the class check, we know that they are an instance of our class. If they are not precisely our class, then they are an instance of a child class, and we should make sure that they are equal to us. A similar change also needs to be made to compareTo.

Of course, if I’ve got a Collection of arbitrary TwoDeePoint objects, I now know that two of them may not be equal even though their x and y values are equal, so if I only care about the x and y values, I need to write my own method to compare them which is external to the TwoDeePoint class, which really begs the question of whether that equals method is worth anything at all…

EDIT: I just realized that my code has a possible infinite recursion case. Specifically, if two ThreeDeePoint objects compare to eachother, they’ll end up spinning infinitely into the abyss. Crap…I don’t know if there is a solution to this problem without breaking its object-oriented nature (e.g.: explicitly checking x and y in ThreeDeePoint)…

Tags: ,