WTF Indeed

This thread on The Daily WTF is rather depressing. Essentially, the original poster is complaining about the following code:

/**
    Set the value of the isRecordLocked attribute.
    @param newValue is the new isRecordLocked value.
*/
public void setIsRecordLocked(Boolean newValue) {
    // Update state if required.
    if (isRecordLocked != null) {
        if (newValue == null || !isRecordLocked.equals(newValue)) {
            context.setDirty(true);
        }
    }
    else if (newValue != null) {
        context.setDirty(true);
    }

    // Change the value.
    isRecordLocked = newValue;
}

He indicates he need to write out a truth table to figure out what it does. So let's do that:

newValue
true false null
isRecordLocked true - dirty dirty
false dirty - dirty
null dirty dirty -

Hmm... I wonder what that's doing... There seems to be a diagonal stripe of "-" indicating setDirty(true) is never called when the two values are equal... So, apparently, setDirty(true) is used to mark the object dirty! Wow!

The post goes on to explain that the if statement could replaced with a "much simpler one-liner." Really?

Well, it can be made into one line easily:

if ((isRecordLocked != null) ? (newValue == null || !isRecordLocked.equals(newValue)) : newValue != null) context.setDirty(true);

Well, it's one line - but I don't think I'd call it cleaner.

So, what's the right answer? Well, several posters came up with:

setDirty(isRecordLocked != newValue);

Which is wrong, because in Java, that checks to see if they aren't the same reference. So something like new Boolean(true) != new Boolean(true) would evaluate to true as they'd be different references. This is obviously not intended.

So we can move on to the next set of wrongness:

setDirty(isRecordLocked.equals(newValue));

Except that in this case, if isRecordLocked is null then a NullPointerException is thrown. Whoops!

So, fixing that, we get our final set of wrong answers:

setDirty(isRecordLocked == null ? newValue != null : !isRecordLocked.equals(newValue));

What's wrong with that? Well, imagine the following:

setIsRecordLocked(Boolean.TRUE);
setIsRecordLocked(Boolean.TRUE);

In that case, there will be two calls: one to setDirty(true) and then a second to setDirty(false) which is almost certainly not intended.

So we have to break it out into an if statement, getting us:

if (isRecordLocked == null ? newValue != null : !isRecordLocked.equals(newValue))
    setDirty(true);

And now it's no longer a one-liner, and it really isn't any clearer to read. My personal favorite solution is the following:

if (!(isRecordLocked == null ?
        isRecordLocked == newValue :
        isRecordLocked.equals(newValue))) {
    setDirty(true);
}

Why? Because it makes it fairly clear we're checking to see if they're not the same value. I suppose you could also do something like:

if (isRecordLocked == null ?
        isRecordLocked != newValue :
        !isRecordLocked.equals(newValue)) {
    setDirty(true);
}

It's questionable which is easier to read. I don't like the idea of switching from equals (isRecordLocked == null) to inequals mid-stream, so I prefer the !(equals pattern) style.

But that's my personal taste.

In any case, this has turned into a real WTF when so many posters in that thread couldn't figure out how to do something fairly simple like determining if two values were equal when either one might be null. And you also have to wonder WTF possessed the poster to think the original was wrong in the first place.

Topics: