Pankaj Kumar's Weblog

A post at THE DAILY WTF includes a function with a compound if statement consisting of eight lines of code to set a flag when the Boolean argument is not equal to a Boolean member, suggesting that the code is correct but overly complex and verbose.

/**
    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;
}

The post also includes a suggestion that the compound if statement can be replaced entirely by a much more understandable one-liner, but doesn't really include the one-liner (perhaps obvious to the submitter).

I struggled for few minutes to figure out the obvious one liner, but couldn't come up with anything signficantly better. The problem is that a simple (A != B) doesn't work for Java Objects (and Boolean is an Object), for either A or B could be null and invoking equals method on null would result in a NullPointerException.

I clicked to see the comments, hoping to find the elusive one-liner in the copious comments (137, when I checked). No luck there. Though some people pointed out that there was nothing wrong with the code, most attempts to better the code resulted in buggy code.

Update (4:40PM, Nov. 30): Got a comment via email(My comments link is disabled due to heavy comment spam and I haven't gotten around to upgrade my blogging software to include an effective filter)

Hi,

I posted this comment to your blog about the thedailywtf code - your
blog software allowed me to preview, and post, then said something
along the lines of "this blog is not accepting comments"..

Cheers.

Ricky.

Because context.setDirty should only be invoked conditionally, and the
guess is that it's a void method, the ternary operator is of no use.
The best I can come up with is:

if (isRecordLocked==newValue || (isRecordLocked != null &&

isRecordLocked.equals(newValue))

   context.setDirty(true);

isRecordLocked=newValue;

It might be possible to sandwich the last line into the if statement
somewhere, but I can't see how to do that while guaranteeing its
execution, and preserving the if statement's results.

If invoking context.setDirty(false) when the condition failed was
acceptable (unlikely), then this would work, still 2 lines though:

context.setDirty(isRecordLocked==newValue || (isRecordLocked != null

&& isRecordLocked.equals(newValue));

isRecordLocked=newValue;

Actually, this replacement is problematic (read, buggy!): it attempts to call context.setDirty(true) when both the member (isRecordLocked) and the argument (newValue)are equal, whereas the original code did it the other way round. Notice the emphasis on attempts -- the attempt fails when isRecordLocked is null and newValue is not ( both isRecordLocked==newValue and isRecordLocked != null &&
isRecordLocked.equals(newValue) will evaluate to false).

Besides, I don't think the replacement is any better in terms of overall readability!

In any case, it just proves my original point -- test of equality, or rather inequality, of two objects is pretty hard in Java.

Update (9:50PM, Dec. 1): Got the following comment, again via email, from the previous commenter Ricky:

You're right, my code was naive. I should probably have written a test that passed with the 'WTF' code, and written my implementation against that. I most likely would have done if it wasn't just for a blog. ;)

"In any case, it just proves my original point -- test of equality, or
rather inequality, of two objects is pretty hard in Java."

Actually, no. Testing equality of objects when all you have are two
references that *may* point at objects is the problem. If you
systematically disallow null (that doesn't necessarily mean testing
for it everywhere), then you're really comparing objects, not just
references, and then you've got this into a 3 liner.

if (!a.equals(b))

{

  setDirty(true);

  a=b;

}

Btw, I did come across a somewhat cleaner solution at behrangsa's page.