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

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
}