An efficient equals(Object o) implementation

Question

I read this SO post after I wrote out the title but still decided to go through with the question on bug-proof implementations of equals in Java. This is my normal implementation

@Override
        public boolean equals(Object o){
            if(o == null) return false;
            if(o instanceof CompositePk == false) return false;
            if(this == o) return true;
            CompositePk that = (CompositePk)o;
            return new EqualsBuilder().append(this.id, that.id)
                                      .append(this.bucketId, that.bucketId)
                                      .isEquals();
        }

using Apache's EqualsBuilder to do the mundane stuff. Even easier than this is my Netbean's automatically generated equals(o) implementation

 @Override
        public boolean equals(Object obj){
        if (obj == null) {
                return false;
            }
            if (getClass() != obj.getClass()) {
                return false;
            }
            final TemplatesWrapper other = (TemplatesWrapper) obj;
            if (this.timeAdded != other.timeAdded && (this.timeAdded == null || !this.timeAdded.equals(other.timeAdded))) {
                return false;
            }
            return true;
    }

I take these from 2 diff projects but they both try to accomplish the same thing but using diff approaches. Which style would you rather or are there any flaws you spot?

Answer 1

I would do it this way:

public boolean equals(Object ob) {
  if (ob == null) return false;
  if (ob == this) return true;

  if (!(ob instanceof MyClass)) return false; // OR
  if (ob.getClass() != getClass()) return false;

  // check relevant members
}

The two lines in the middle are different. One allows for subclasses to be equal (the first one), the other doesn't. Use whichever one is appropriate.

To give you an example, Java's AbstractList class will probably use the second form, because the exact implementation of List is irrelevant. what matters is if the members are equal and in the same position.

Conversely, a Person class should use the first form (instanceof) because if there is a Student subclass and you call Person.equals(Student) it may return true without checking the extra fields in Student whereas Student.equals(Person) will probably return false. If equals() isn't commutative, you're asking for trouble.

I tend to use equals() methods generated by my IDE (IntelliJ IDEA) rather than creating an unnecessary dependency to some Apache library for little gain.

Answer 2

Apache's is better than yours or cletus'.

As far as my vague memory suggests, there is a problem with using instanceof in equals; I can't quite put my finger on why yet, perhaps someone will elaborate. I could be wrong.

-- Edit:

As Chris and Steve helpfully explain below, I was thinking of the "symmetric property" of equals implementation. On this basis, I can back up my claim of prefering the Apache implementation :)

Answer 3

First of all, there's no need to test for null, then test for instanceof, since foo instanceof Bar evaluates to false when foo is null.

It's weird to compare the result of the instanceof operator to false, since instanceof is a boolean operation.

Comparing classes with getClass() is at best controversial. Joshua Bloch, who wrote much of the Java collections framework and a lot of other important stuff besides, says

This technique ("getClass-based equals methods") does satisfy the equals contract, but at great cost. The disadvantage of the getClass approach is that it violates the "Liskov Substitution Principle," which states (roughly speaking) that a method expecting a superclass instance must behave properly when presented with a subclass instance. If a subclass adds a few new methods, or trivially modifies behavior (e.g., by emitting a trace upon each method invocation), programmers will be surprised when subclass and superclass instances don't interact properly. Objects that "ought to be equal" won't be, causing programs to fail or behave erratically. The problem is exacerbated by the fact that Java's collections are based on the equals method.

You should use instanceof instead of comparing via getClass() unless you have some specific technical reason not to.

After establishing that the other object is comparable to this, you then compare primitives with == and objects with equals. It's more complicated if any of your member objects can be null; you must then write verbose clauses to compare possibly null things to each other (or write a bothNullOrEqual(Object a, Object b) method).

The EqualsBuilder approach looks bogus to me, but that's just a "smell", which I won't argue against technically. In general, I don't like extra method calls in a method that may be called frequently.

The Apache one is bogus because it tests for null and uses the getClass() comparison.

Here's mine:

@Override
public boolean equals(final Object o) {
    if (!(o instanceof MyClass))
        return false;
    final MyClass om = (MyClass)o;
    // compare om's fields to mine
}

Answer 4

Honestly, the less code you have to write, the better off you are (in most cases).

The code that's generated has been debugged and used by many MANY people. You might as well use what's generated (and if you need to enhance the performance, do so).

The advantage of using the generated code: any time your instance fields changes (and this generated code wasn't modified), you can simply regenerate code.

Sometimes, it's easier to think about maintainability. Rule of thumb: the less code you write yourself, the less you have to debug. If the generated code doesn't take a huge performance hit, generate it!