write(this); //not that

Halloween is tomorrow, so I thought I’d share some of the scariest code that I’ve seen in awhile. Behold…

public class Something  implements java.io.Serializable {
    private static final long serialVersionUID = 1L;
    private boolean __hashCodeCalc = false;
    private Object __equalsCalc = null;

    /**
     * @param obj
     */
    public synchronized boolean equals(java.lang.Object obj) {
        if (!(obj instanceof Something)) return false;
        if (obj == null) return false;
        if (this == obj) return true;
        if (__equalsCalc != null) {
            return (__equalsCalc == obj);
        }
        __equalsCalc = obj;
        boolean _equals= true;
        __equalsCalc = null;
        return _equals;
    }

    /**
     * @return int
     */
    public synchronized int hashCode() {
        if (__hashCodeCalc) {
            return 0;
        }
        __hashCodeCalc = true;
        int _hashCode = 1;
        __hashCodeCalc = false;
        return _hashCode;
    }

    // ... rest of class omitted for brevity
}

Where do I start?

The first thing that caught my eye is the synchronized equals() and hashCode() methods. “Why would they be synchronized?”, I wondered. After all, there’s no point in synchronizing them unless they alter some shared state. Oh, there it is…they alter the __equalsCalc and __hashCodeCalc instance variables. That’s why they’re synchronized. Gotcha. Then it makes perfect sense to synchronize those methods. Very good then.

Hold on a minute…what are those instance variables for???

It seems (at least on the surface) that the these methods are trying to shortcircuit any future calls by stashing away whatever it was being compared with. I think. It’s really hard to tell what’s going on in there, so I’m just speculating at this point. Bear with me…this gets scary.

Please refrain from beating your computer with a baseball bat…I promise the code won’t jump out and bite.

Anyhow, there’s a lot of gnarly comparison logic going on in equals(). Obviously if the thing passed in is of a different type, then they’re not equal. If the thing passed in is null…also not equal. If the thing being passed in is the exact same thing as this object, then they are equal. Good so far.

Now, if the formerly stashed away copy of some object is not null and is the exact same object as the object passed in, then obviously they’re equal. It takes a bit of brain twisting to believe this is true, but it should be true because the only way that __equalsCalc would ever be set is if in some past invocation the object passed in were determined to be equal to the current object.

The final part of equals() is dedicated to setting the __equalsCalc method…sorta. At this point in the code we still don’t know if the values are equal, but we do want to stash away the object for future reference. So, we set the __equalsCalc to the object passed in. Then we set _equals to true. Then it gets weirder…we set the __equalsCalc to null to guarantee that it will always be null upon entry to equals(). Then equals() wraps things up by returning the value of _equals which, by the way, is always true.

Seriously folks…I’m not making this stuff up. This came from real production code (class name changed to protect the guilty).

Meanwhile, the hashCode() method goes through some similar nonsense. To summarize, if the __hashCodeCalc is true, then we’ll return 0. Otherwise, we’ll return 1, but not before setting __hashCodeCalc to true and then setting it back to false, guaranteeing that __hashCodeCalc will never be true upon reentry to this method. Therefore, this method will ultimately always return 1.

Oh, the horror!

And although it seems nitpicky at this point, I should mention that the JavaDoc comments are completely useless.

With new understanding of what these methods really do and after applying some refactoring to clean up the mess, here’s what I ended up with.

public class Something  implements java.io.Serializable {
    private static final long serialVersionUID = 1L;

    public boolean equals(java.lang.Object obj) {
        return obj instanceof Something;
    }

    public int hashCode() {
        return 1;
    }

    // ... rest of class omitted for brevity
}

Even though it’s cleaner, I’m still not so sure that there’s much value in these implementations of equals() and hashCode(). It’d certainly break the current behavior, but I strongly suspect that a real implementation of equals() that compares members of the class and a real hashCode() method that truly calculates a hash code would be more desirable. Perhaps something like Apache Commons Lang’s EqualsBuilder and HashCodeBuilder would come in handy.

In fairness to the project that I borrowed this code from, this code was actually generated using an old version of Apache Axis’ WSDL2Java tool. No humans involved the project are to blame for unleashing this monster.