Null check in .equals implementation

I have inherited some legacy code that I'm now trying to clean up.

I feel like the 2nd null check is redundant, see comments:

private java.lang.Object __equalsCalc = null;
    public synchronized boolean equals(java.lang.Object obj) {
        if (!(obj instanceof ArrayOfColumn)) return false; // Here is first check if the obj is not null, if there is instaceof ArrayOfColumn then it's not null
        ArrayOfColumn other = (ArrayOfColumn) obj;
        if (obj == null) return false; // Therefore this check is redundant and "return false" will never execute, so it's dead code
        if (this == obj) return true;
        if (__equalsCalc != null) {
            return (__equalsCalc == obj);
        }
        __equalsCalc = obj;
        boolean _equals;
        _equals = true && 
            ((this.column==null && other.getColumn()==null) || 
             (this.column!=null &&
              java.util.Arrays.equals(this.column, other.getColumn())));
        __equalsCalc = null;
        return _equals;
    }

Anything else?

First up, yes, you're right that the second null check is redundant. If obj is null then the method will return false on the first check:

if (!(obj instanceof ArrayOfColumn)) return false;

Of course, that would better be written:

if (!(obj instanceof ArrayOfColumn)) {
        return false;
    }

But, that's the least of my concerns. The synchronized equals is probably there because other methods are synchronized, but synchronization comes at a cost. Unless you are sure you need it, remove it.

Additionally, synchronized methods are seldom the best solution. It is normally better to have tighter control of your locks so that nobody but you can hold them. That normally means using a private, internal "monitor" for synchronization:

private final Objct lock = new Object();

private boolean equals(Object obj) {
    synchronized(lock) {
        .....
    }
}

Using the private lock prevents other people from hanging your application by using your class as their own monitor. Imagine if your class is called MyClass, and someone does:

private MyClass instance = new MyClass();

 synchronized(instance) {
     Thread.sleep(10000);
 }

If they did the above, your methods in other threads would never run, and your application would "hang".

Now, abut the __equalsCalc variable. It does nothing useful. It is set, and then cleared, inside the synchronized method, and, as a result, it will never, ever have any value in it that is useful. It is completely redundant and a waste of time.

Oh, and what's with the fully qualified java.lang.Object? Just use Object.

I would rewrite the method as (using the same synchronized method to be compatible with other methods in your class):

public synchronized boolean equals(Object obj) {
    if (!(obj instanceof ArrayOfColumn)) {
        return false;
    }
    if (obj == this) {
        return true;
    }
    ArrayOfColumn other = (ArrayOfColumn) obj;
    if (getColumn() == null) {
        return other.getColumn() == null;
    }
    return other.getColumn() != null && Arrays.equals(column, other.getColumn());
}

Note that you still have a synchronization vulnerability - the other column's column could be changed between checking for whether it's column is null, and using the column in the equals. You could still get null pointer exceptions if someone changes the other column in the middle of your equals. You should consider synchronizing on that other column too:

public synchronized boolean equals(Object obj) {
    if (!(obj instanceof ArrayOfColumn)) {
        return false;
    }
    if (obj == this) {
        return true;
    }
    ArrayOfColumn other = (ArrayOfColumn) obj;
    synchronized(other) {
        if (getColumn() == null) {
            return other.getColumn() == null;
        }
        return other.getColumn() != null && Arrays.equals(column, other.getColumn());
    }
}

Unfortunately, cross-synchronizing like that can lead to deadlocks if you are not careful.

StackExchange Code Review Q#102669, answer score: 14