Should i continue shortening my equals and hashcode functions?

Question

When i have a class like this

public class MyClass {
    private ClassA classA;
    private ClassB classB;
}

my equals and hascode function often end up in a mess like this

@Override
public int hashCode() {
    final int prime = 31;
    int result = 1;
    result = prime * result + ((classB == null) ? 0 : classB.hashCode());
    result = prime * result + ((classA == null) ? 0 : classA.hashCode());
    return result;
}

@Override
public boolean equals(Object obj) {
    if (this == obj)
        return true;
    if (obj == null)
        return false;
    if (getClass() != obj.getClass())
        return false;
    MyClass other = (MyClass) obj;
    if (classB == null) {
        if (other.classB != null)
            return false;
    } else if (!classB.equals(other.classB))
        return false;
    if (classA == null) {
        if (other.classA != null)
            return false;
    } else if (!classA.equals(other.classA))
        return false;
    return true;
}

So i started to ignore that someone will compare apples with bananas. HashSets or HashMaps in my applications always contains the same object types... i never created a List and started mixing Integer with String.

@Override
public int hashCode() {
    return classB.hashCode();
}

@Override
public boolean equals(Object obj) {
    MyClass other = (MyClass) obj;
    return other.classB.equals(this.classB) && other.classA.equals(this.classA);
}

is it bad or good practice to ignore uncommon cases and let the exceptions throw? I think it is most commonly an error when complete different classes are compared.

Answer 1

It's extremely bad practice to let equals() or hashCode() throw exceptions (I'd encourage you to read Effective Java for the details).

Also, your methods are unnecessarily complex. Ever since Java 7, this is pretty much the canonical way of writing these methods:

@Override
public boolean equals(Object o) {
    if (this == o) {
        return true;
    }else if (o instanceof MyClass) {
        // some will argue that the line above should have a
        // .getClass() check instead of an instanceof check.
        // those people also need to read Effective Java :-)
        MyClass that = (MyClass) o;
        return Objects.equals(this.classA, that.classA)
            && Objects.equals(this.classB, that.classB)
    } else {
        return false;
    } 
}

@Override
public int hashCode() {
    return Objects.hash(classA, classB);
}

Answer 2

It is bad practice how you shortened your equals() method (your hashCode method disregards classA, but there might be use cases where this is appropriate). Let me explain the problem with equals.

Your apparent approach is to assume that all Objects are of type MyClass. But Java is a type-safe language, so you should always define the data type you expect. Otherwise, you could simply everywhere expect Objects and cast them as you did in your example. That will probably work just as it does with scripting languages (e.g., Javascript) but once your code base becomes larger and multiple developers try to understand and improve your code, it becomes confusing and you loose the benefit of type-safety (e.g., to see the type-related errors during the compile time).

The other problem you are facing here is that you are using the interface of Object. But let's assume you define an additional (overloaded) method using the correct data type, your code would then look like this:

public boolean equals(MyClass other) {
    return other.classB.equals(this.classB) && other.classA.equals(this.classA);
}

Now you have a method that exactly does what you expect it to do, right? Not completely:

MyClass myClass1 = new MyClass();
MyClass myClass2 = clone(myClass1)
myClass1.equals(myClass2) // works => true
myClass2.equals(myClass1) // the same
Object myClass3 = (Object) myClass2
myClass3.equals(myClass1) // Compares the address with the default Object.equals() => false
myClass1.equals(myClass3) // the same

You see that just by changing the data type, the result changes, although myClass1, myClass2, and myClass3 are deeply equal. You will not get an exception or compilation error. Well, let's change that by actually overriding the method (just as you did).

@Override
public boolean equals(Object obj) {
    MyClass other = (MyClass) obj;
    return this.equals(other); // this is the overloaded method
}

Now, you get a ClassCastException if obj is not of type MyClass. This works as long as you know how you implemented it, because there is no Throwable in the signature. One more thing to consider is that the ClassCastException extends RuntimeException which is unexpected for a developer (i.e., unchecked). That means if I don't know how it is implemented (e.g., because it is part of a compiled library), or even if it was you who continues the development years later and don't remember it, one may come up with something like this:

public class MyExtraClass extends MyClass {
    private String someIrrelevantStuff;
    // I don't want or need a separate equals
    // and assume that MyClass or Object takes care of it
}

MyClass myClass1 = new MyClass();
MyClass myClass2 = (MyClass) new MyExtraClass();
myClass2.equals(myClass1) // works => false
myClass1.equals(myClass2) // Throws ClassCastException
// Application crashes completely at this point
// just because I didn't expect the unchecked exception

I would say, this example (i.e., subclassing) is not uncommon or an error, but it fails. If you added the exception to the signature, it would be overloaded again and you are back to the point where you started. You see that there is no way out just because of the Object's interface.

However, it is justified to ask if this interface is still suitable for Java in its current release version. Wouldn't it be appropriate to provide a default deepEquals() method instead of the current equalsAddressOf() (not to confuse with the current deepEquals() which is just applicable for arrays)? Scala already tackles this concern with its case classes. You may consider switching to Scala or an (optionally) unsafe language like Python if you want shorter codes.

If you really preferred a distinction of the false state, after all. You could simply use your own interface:

public abstract class DeeplyComparableObject extends Object {

    public abstract boolean deeplyEquals(Object o) throws TypeNotEqualException;
}

public class TypeNotEqualException extends Exception {

}