The Daily WTF: Curious Perversions in Information Technology

2006-11-29 Reply Admin

Wow, of soooo many posts so far, there has only been ONE brief and concise oneliner check - Jon Haugsand that proposed the static Object.equals(o1, o2). Two more posts didn't have actual errors, but were kinda like stealing the idea. Unneeded... First someone just inlined that static inside the code. Way to go. And then someone used the same static, only in a separate ObjectUtils class instead of Object, adding unneeded setDirtyDifferent crap. Got it correct from the second try, atleast.

A couple more correct corrections used code not as brief as Jon's, and came after he had posted it too. Why did this thread not STOP after the solution was given? Why did the wrong solutions not stop atleast? WTF?

2006-11-29 Reply Admin

/**
   Set the value of the isRecordLocked attribute.
   @param newValue is the new isRecordLocked value.
*/
public void setIsRecordLocked(Boolean newValue) {
   // Set dirty flag if the new value is not the same as the current value
   context.setDirty( isRecordLocked != newValue);


   // Change the value.
   isRecordLocked = newValue;
}

2006-11-29 Reply Admin

Ottokar:
You have a database. The DB has tables. A table can have booleans. And they can have three states (NULL, true, false). So your system has to handle the tristate DB-Values.

Of course not!

They can have four states: true, false, FileNotFound, NULL.

2006-11-29 Reply Admin

I code here I've generalised the equalsTristateBoolean to one that takes Objects

public static boolean equals(Object a, Object b)
{
    return ( (a == null && b == null) || (a != null && b != null && a.equals(b))
}

I've generally not bothered with b!=null in the second test as I expect a.equals(b) to handle that case. Maybe the extra check saves a method call.

Captcha was "Java".

2006-11-29 Reply Admin

The original code never calls setDirty with false as its argument. This snippet would set dirty to false depending on the equality or otherwise of the old and new values. This isn't functionally equivalent to the original.

2006-11-29 Reply Admin

How about:

public void setIsRecordLocked(Boolean newValue) {
    // Call setDirty if old and new values are different
    if (newValue != isRecordLocked  && (newValue == null || !newValue.equals(isRecordLocked))) {
        context.setDirty(true);
    }
    isRecordLocked = newValue;
}

One liner and seems to be correct.

ParkinT · 2006-11-29 Reply Admin

Tim Gallagher:

the if statements can be replaced entirely by a much more understandable one-liner."

But, if you are being paid by SLOC...

I work on a US Air Force project and many of the contractors working on this System pay their coders based on lines of code. Not a very good way to reward efficient coding style but, at least, it is a consistent way to measure.

2006-11-29 Reply Admin

I think you are all missing the point, if you take pen and paper and write a table with all possible conditions, you will see that actually setDirty(true) is called every time except if both newValue and isRecordLocked are null !!!

So the correct code snippet would be:

public void setIsRecordLocked(Boolean newValue) { if((newValue==null) && (isRecorLocked==null)) { setDirty(true);

}

// Change the value.

isRecordLocked = newValue;
}

Whether this is what the function was meant to do or not, I can't tell...

2006-11-29 Reply Admin

Look at it this way.

Nullable boolean types are necessary to answer inane illogically-formed questions.

For example:

Have you stopped beating your wife?

A lot of people would like to reach for the value null right there.

Case closed.

2006-11-29 Reply Admin

Anonymous:
Ottokar:
You have a database. The DB has tables. A table can have booleans. And they can have three states (NULL, true, false). So your system has to handle the tristate DB-Values.

Of course not!

They can have four states: true, false, FileNotFound, NULL.

Don't forget to add 'yellow'.

pauluskc · 2006-11-29 Reply Admin

Aha! This is what I was saying the whole time I read thru the thread and read the other examples. I second your notion! Can't change the value of that setDirty stuff because this could be one of dozens of checks. Different needs at the time could require more or less checks or different checks as different objects have different properties. So there could also be setIsRecordChanged() and setIsRecordInvalid() and others like that. By having this function, especially if it's part of a master (top-level) class (gosh, my OO terminology is not that great), instead of a one-liner, it's reusable and can transcend to children and etc. Remember "public" in the decleration. Maybe the security is WTFey.

Without having other code around it, it could be classifiable as a WTF, IMHO.

pauluskc · 2006-11-29 Reply Admin

Ding! We have the answer.

2006-11-29 Reply Admin

Nothing about this code is a WTF. The real WTF is that in order to think this is a WTF one would have to been limited to developing in Javascript using pre-built frameworks. Just because someone with little knowledge says something is stupid doesn't mean you don't have to think about it for yourself.

2006-11-29 Reply Admin

I had to use a pen and paper to work out exactly what conditions the setDirty(true) call was made.

I found the real WTF.

Benanov · 2006-11-29 Reply Admin

Anonymous:
Look at it this way.

Nullable boolean types are necessary to answer inane illogically-formed questions.
For example:
Have you stopped beating your wife?
A lot of people would like to reach for the value null right there.
Case closed.

I prefer throw new ArgumentNullException("Value of Wife is NULL");

but that's just me.

rmr · 2006-11-29 Reply Admin

Anonymous:
I had to use a pen and paper to work out exactly what conditions the setDirty(true) call was made.

I found the real WTF.

Yeah! Man that is messed up - a programmer using paper to wrap their head around something!

. . .

Seriously, where did the attitude that programmers should do everything in their heads come from?

emurphy · 2006-11-29 Reply Admin

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

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

  || (!isRecordLocked.equals(newValue)) ) {

    context.setDirty(true);

Assumptions:

The system has a legit reason for using the nullable 'Boolean' class rather than the 'boolean' primitive.
The conditional logic is correct, i.e. setDirty(true) unless (1) they're equal according to .equals() or (2) they're both null.
The usage of setDirty() is correct, i.e. if something else did setDirty(true) before us then (1) doing a second setDirty(true) is harmless, but (2) we must not step on that result by doing setDirty(false). This second point rules out all setDirty(conditional) solutions.
Short-circuit evaluation applies. If it doesn't (i.e. if this is an obfuscation of some other language where it doesn't), then we must restrict evaluation of the equals() part via if/else.
Clarity is more important than cleverness. Although I think all the clever one-liners are already ruled out by one or more of the above.

2006-11-29 Reply Admin

Anonymous:
Anonymous:
Ottokar:

You have a database. The DB has tables. A table can have booleans. And they can have three states (NULL, true, false). So your system has to handle the tristate DB-Values.
Of course not!

They can have four states: true, false, FileNotFound, NULL.
Don't forget to add 'yellow'.

You guys are slipping - it took 24 posts before someone even mentioned file-not-found

2006-11-29 Reply Admin

if (newValue != isRecordLocked) {

context.setDirty(true);

isRecordLocked=newValue;

}

2006-11-29 Reply Admin

mmm. never mind, I missed an implication.

2006-11-29 Reply Admin

What I got, from working out the 'before' and 'after' values of these variables in excel is:

public void setIsRecordLocked(Boolean newValue) {

if (isRecordLocked != newValue) {

context.setDirty(true);

}

isRecordLocked = newValue;

}

The only time context.setDirty() remains uncalled (and I assume 'dirty' is unchanged) is when isRecordLocked == newValue. In every other possible outcome, dirty is set to true.

Ghost Ware Wizard · 2006-11-29 Reply Admin

that ranks right up there with creating an object with functions in it to get a reserved type.

e.g.

public static class ObjectVariables
{

    ObjectVariables
    {
    }

    public static int INT()
    {
        int nRetVal = -1;
        return nRetVal;
    }

    public static double DOUBLE()
    {
        double dblRetVal = -1;
        return dblRetVal;
    }

}

JamesCurran · 2006-11-29 Reply Admin

Anonymous:
I think you are all missing the point, if you take pen and paper and write a table with all possible conditions, you will see that actually setDirty(true) is called every time except if both newValue and isRecordLocked are null !!!

That's just not true. Consider the case newValue & isRecordLocked are both false.

if (isRecordLocked != null) {
This is true, so we continue.

if (newValue == null ||
This is false so we evaluate the other side of the OR

|| !isRecordLocked.equals(newValue)) {
This is also false, so we exit the if() block.

else if (newValue != null) { context.setDirty(true); }
Since the first if() was true, we skip the else clause (did you mistakenly associate it with the second if()?)

isRecordLocked = newValue;
Finally, we pointless set a value which is already false to false.

JamesCurran · 2006-11-29 Reply Admin

Shadowman :
The only time context.setDirty() remains uncalled (and I assume 'dirty' is unchanged) is when isRecordLocked == newValue. In every other possible outcome, dirty is set to true.

As others have pointed out, == is only true for two references to the same object, but not for two distinct objects which just happen to have the same value. To compare to separate objects, you have to use the Boolean.equals method, but to use that, you have to make sure that the calling object is not null.

Maurits · 2006-11-29 Reply Admin

I find that the main source of confusion in snippets like these is not the length of the code, but the negative assertions. One-liners don't tend to make things clearer. Here's my refactoring of the code to use positive assertions, as well as some (hopefully) better comments:

public void setIsRecordLocked(Boolean newValue) {

    if (newValue == null && isRecordLocked == null) {
        // no change; was null, still is
    } else if (newValue == null || isRecordLocked == null) {
        // change to or from null
        isRecordLocked = newValue;
        context.setDirty(true);
    } else if (newValue.equals(isRecordLocked)) {
        // no change; either a) was true, still is or b) was false, still is
    } else {
        // change from true to false or vice versa
        isRecordLocked = newValue;
        context.setDirty(true);
    }
}

2006-11-29 Reply Admin

public void setIsRecordLocked(Boolean newValue) {
    if (!(isRecordLocked == null ?
            isRecordLocked == newValue :
            isRecordLocked.equals(newValue))) {
        context.setDirty(true);
    }
}

Simple, effective, and makes the intention much clearer.

Note that the "isRecordLocked == newValue" could just as easily be "newValue == null" but using "isRecordLocked == newValue" makes the intention clearer.

Null handling like the above comes up all the time in Java and the above pattern is repeated all over the Sun Java library implementation.

2006-11-29 Reply Admin

Ok, I think this should be functionally identical to the original:

public void setIsRecordLocked(Boolean newValue) {
    // Update state if required.
    if (  !(newValue == null && isRecordLocked == null) && !(newValue != null && newValue.equals(isRecordLocked))   )  {
        context.setDirty(true);
    }
    // Change the value.
    isRecordLocked = newValue;
}

Also, the 'isRecordLocked = newValue;' could almost certainly be moved inside the if statement.

The only way that would cause errors is if there is some WTF code elsewhere that compares the references instead of using .equals().

2006-11-29 Reply Admin

Anonymous:
public void setIsRecordLocked(Boolean newValue) {
    if (!(isRecordLocked == null ?
            isRecordLocked == newValue :
            isRecordLocked.equals(newValue))) {
        context.setDirty(true);
    }
}
Simple, effective, and makes the intention much clearer.
Note that the "isRecordLocked == newValue" could just as easily be "newValue == null" but using "isRecordLocked == newValue" makes the intention clearer.
Null handling like the above comes up all the time in Java and the above pattern is repeated all over the Sun Java library implementation.

Hmm, I could've sworn that 'null == null' returned false. Learn something new everyday :)

On the other hand, you forgot to set isRecordLocked to newValue.

Mkamba · 2006-11-29 Reply Admin

No, no! You guys have got it all wrong! The real WTF has nothing to do with the code, but with the moral.

Think about this for a second. You're hanging around some place (coffee shop, bar, produce department) and a member of the appropriately attractive gender walks up to you and says "setDirty( true )." How many of us would really withold even our spam email adress after that?

VGR · 2006-11-29 Reply Admin

Hoo boy. Yes, lots and lots of wrong answers here.

First, I can only address this as a Java issue, though it wouldn't surprise me if the real code is not in Java but was turned into Java as part of Alex's anonymization process. Since I have no way to know, I'll just pretend it really is in Java.

Second, the use of the == and != operators is not acceptable, not even in Java 1.5 or 1.6 where auto(un)boxing is in use. You can see this by executing this code:

System.out.println(Boolean.TRUE == new Boolean(true));

Third, it is always permitted to pass null to the equals method.

So, the logic here is: if newValue differs from isRecordLocked, call setDirty(true).

My version:

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

2006-11-29 Reply Admin

if (isRecordLocked == null) {

if (newValue == null) setDirty(true);
} else {

if (isRecordLocked.equals(newValue)) setDirty(true);

}

Not that clean, but more readable than the original (I think).

2006-11-29 Reply Admin

rmr:
Anonymous:
I had to use a pen and paper to work out exactly what conditions the setDirty(true) call was made.

I found the real WTF.
Yeah! Man that is messed up - a programmer using paper to wrap their head around something!
. . .
Seriously, where did the attitude that programmers should do everything in their heads come from?

There's nothing wrong with using pen and paper to analyze code in and of itself. However, if you need pen and paper to evaluate something this simple, you're going to have problems when you come to something more than a few lines long. It's like needing paper to evaluate 4 * 5 -- if the basics trip you up, the hard stuff is going to kill you.

On another note, I'm rather surprised by the number of people failing to grasp the code and failing horribly in their attempts to correct or simplify it. I can understand if Java isn't your language, but if it is...

2006-11-29 Reply Admin

This particular example is in Java, and in Java a boolean and a Boolean have this important distinction: a boolean is not an Object. Because it is not an object, you can't use a primitive boolean as a parameter to any method that takes an Object, which means you can't use a boolean in a Collection or Map. Thus there's Boolean, an immutable wrapper around a true or false value. This is called "boxing," which I'm sure most of you know but I'm putting it out there because there seems to be some confusion about it.

In an application that deals extensively with Collections and Maps -- such as pretty much any ORM system -- you're likely to have more boxed primitives than actual primitives, and to unbox and rebox them before each method call would be kind of stupid. In such an application, you'd see a lot of method signatures looking for the boxed versions of Booleans -- not because you need ternary logic, but because you already have the object.

I have no problem whatsoever with this code. Draw out the truth table and you find that we want to dirty the record when both aren't null AND both aren't equal. That's exactly what this code is testing, while being careful not to call a method on a null isRecordLocked object.

Okay, there is an unnecessary test of a null newValue which I'm sure you can chock up to a misunderstanding of the equals() contract. But the real WTF here is why Carl Cerecke thinks context.setDirty( (isRowLocked != null && !isRowLocked.equals(newValue)) || newValue != null ) is more legible. You don't make better code by condensation!

tibo · 2006-11-29 Reply Admin

Tim Gallagher:
It was no real surprise that the if statements can be replaced entirely by a much more understandable one-liner."

Yeah, that's true, but the one-liner would messier. There's absolutely nothing wrong with the code snippet (other than manipulating the static variable "context" like this).

2006-11-29 Reply Admin

Anonymous:
if (isRecordLocked == null) {
if (newValue == null) setDirty(true);
} else {
if (isRecordLocked.equals(newValue)) setDirty(true);
}
Not that clean, but more readable than the original (I think).

You are a special kind of idiot for fucking this up.

2006-11-29 Reply Admin

I am a machine, you insensitive clod!

2006-11-29 Reply Admin

Anonymous:
Okay, there is an unnecessary test of a null newValue which I'm sure you can chock up to a misunderstanding of the equals() contract. But the real WTF here is why Carl Cerecke thinks context.setDirty( (isRowLocked != null && !isRowLocked.equals(newValue)) || newValue != null ) is more legible. You don't make better code by condensation!

Plus that code would likely be wrong: it would unset the dirty flag when setting isRecordLocked to its current value.

2006-11-29 Reply Admin

If you really really want the setDirty logic in one line:

public void setIsRecordLocked(Boolean newValue) {
    if( ( isRecordLocked != null && !isRecordLocked.equal( newValue ) ) || ( isRecordLocked == null && newValue != null ) ) context.setDirty( true );
    isRecordLocked = newValue;
}

2006-11-29 Reply Admin

Anonymous:
Anonymous:
Okay, there is an unnecessary test of a null newValue which I'm sure you can chock up to a misunderstanding of the equals() contract. But the real WTF here is why Carl Cerecke thinks context.setDirty( (isRowLocked != null && !isRowLocked.equals(newValue)) || newValue != null ) is more legible. You don't make better code by condensation!
Plus that code would likely be wrong: it would unset the dirty flag when setting isRecordLocked to its current value.

Good catch. I bow to the superior intelligence. It's got to be a minimum two-liner (if statement, value set) testing a minimum of three booleans.

tibo · 2006-11-29 Reply Admin

Anonymous:

If you really really want the setDirty logic in one line:

public void setIsRecordLocked(Boolean newValue) {
    if( ( isRecordLocked != null && !isRecordLocked.equal( newValue ) ) || ( isRecordLocked == null && newValue != null ) ) context.setDirty( true );
    isRecordLocked = newValue;
}

Wrong: if newValue is null this will throw a NullPointerException.

2006-11-29 Reply Admin

From Boolean.equals API:

"Returns true if and only if the argument is not null and is a Boolean object that represents the same boolean value as this object."

No null pointer exception.

2006-11-29 Reply Admin

Anonymous:

If you really really want the setDirty logic in one line:

public void setIsRecordLocked(Boolean newValue) {
    if( ( isRecordLocked != null && !isRecordLocked.equal( newValue ) ) || ( isRecordLocked == null && newValue != null ) ) context.setDirty( true );
    isRecordLocked = newValue;
}

Come on, Jalopify your shit, man! It would really be:

public void setIsRecordLocked(Boolean newValue) {
    if( ( isRecordLocked != null && !isRecordLocked.equal( newValue ) ) 
        || ( isRecordLocked == null && newValue != null ) ) 
        context.setDirty( true );
    isRecordLocked = newValue;
}

So much more legible! So much more clear! I love the code snippet of the day!

PS: in Perl you can do all this with a single character. It's Unicode 0x25EC, provided that it doesn't come after a bang, hash, crack, semi-colon or the word "bindle." I don't know why you fuckers aren't using it.

tibo · 2006-11-29 Reply Admin

Anonymous:
From Boolean.equals API:
"Returns true if and only if the argument is not null and is a Boolean object that represents the same boolean value as this object."
No null pointer exception.

I stand corrected.

2006-11-29 Reply Admin

Okay on a WTF scale of 1-10 this ranks about 3. Its just some poorly structured code which I happen to see everyday.

2006-11-29 Reply Admin

First ObjectUtils already exists in the jakarta commons-lang library, and it is better to reuse that than write it yourself. Second, my point was not to reduce the code to an uber-leet single liner. This kind of check probably happens throughout the object as any attribute change would probably mean you need to save it out or something like that. So my solution was intended to reduce this duplication to one line per attribute. Of course proxies/AOP may be a better way to do this, but would be a huge complication.

2006-11-29 Reply Admin

Anonymous:
Come on, Jalopify your shit, man!

It ceases to be a one liner, which was the implied challenge of the WTF post.

I wouldn't write my code this way. It was deliberate.

2006-11-29 Reply Admin

Anonymous:
First ObjectUtils already exists in the jakarta commons-lang library, and it is better to reuse that than write it yourself.

Not neccessarily. For one thing, we've been bit on the ass here numerous times by shoddy, amateurish code from the jakarta commons project (funny how so many shitty hackers are giving their work away for free). For another, we may not want an entire library to be required everywhere our object goes (let's say it's a web service, or an applet, or J2ME). Finally, seems like a whole lot of fucking work just to test a handful of local booleans.

Anonymous:

Second, my point was not to reduce the code to an uber-leet single liner. This kind of check probably happens throughout the object as any attribute change would probably mean you need to save it out or something like that. So my solution was intended to reduce this duplication to one line per attribute. Of course proxies/AOP may be a better way to do this, but would be a huge complication.

You're right about that -- and it would be functionally equivalent to this code only much slower, assuming it were functional. If it weren't functional, well we'd be stuck because even if we were able to find the defect, we wouldn't be able to fix it in code. We'd have to patch the post AOP compilation class, all for what? To eliminate an obvious, clearcut, understandable pattern and replace it with a buzzword capable cycle-waste?

I'll have my pizza with extra Enterprise, please.

rmr · 2006-11-29 Reply Admin

Anonymous:
rmr:
Anonymous:
I had to use a pen and paper to work out exactly what conditions the setDirty(true) call was made.

I found the real WTF.
Yeah! Man that is messed up - a programmer using paper to wrap their head around something!
. . .
Seriously, where did the attitude that programmers should do everything in their heads come from?

There's nothing wrong with using pen and paper to analyze code in and of itself. However, if you need pen and paper to evaluate something this simple, you're going to have problems when you come to something more than a few lines long. It's like needing paper to evaluate 4 * 5 -- if the basics trip you up, the hard stuff is going to kill you.
On another note, I'm rather surprised by the number of people failing to grasp the code and failing horribly in their attempts to correct or simplify it. I can understand if Java isn't your language, but if it is...

But it isn't simple. . . that's why it is on TDWTF.

I think that is a big part of the problem here. Programmers aren't willing to admit that they "need pen and paper". If the original programmer had bee smart enough to realize that he needed to analyze what this function was doing a little bit more, this entry wouldn't exist!

2006-11-29 Reply Admin

Attempt at the one liner (Although, this is about as readable as the way he did it.)

         if(
            (isRecordLocked != null && !isRecordLocked.equals(newValue)) ||
            (newValue != null && !newValue.equals(isRecordLocked)) ||
            ((isRecordLocked = newValue).valueOf("false"))
            )
        {
            context.setDirty(true);
        }

basically, context.setDirty(true) only occurs if isRecordLocked != newValue. I put two comparisons in the if statement, so that whichever one wasn't null would be compared to the other, and then verified inequality. I didn't put in a case for when they're both null, because it's redundant.

I then added a third line which nests the isRecordLocked = newValue into the if statement (thereby having a "one-liner" with two seperate assignment statements - cheap workarounds, ahoy!), using the ().valueOf("false") because that's the only way the compiler would allow that assignment inside the conditional area. I designed it so the overall condition is if ("we should execute this code") || false ) so that the assignment at the end didn't change the logic for going into the if statement's code block.

-Alex

tchize · 2006-11-29 Reply Admin

Tim Gallagher:
I had to use a pen and paper to work out exactly what conditions the setDirty(true) call was made. It was no real surprise that the if statements can be replaced entirely by a much more understandable one-liner."
/**
    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;
}
Moral of the story? Do not try to use this code as a pick up line.

Poor thing. This is a typical null safe equality check in OO (if both null | ( first not null & first equals second)).

You could make it if ( ( isRecordLocked == newValue == null) || ( ( isRecordLocked!=null) && (isRecordLocked.equals(newValue))))

But it's quite less easy to read. Two if inside each other is the most readable way. Nothing wrong with code, and if you need a pencil an paper to understand this... Wow, that's a wtf.

Clearly a code that has nothing to do here.

setDirty( true )

Leave a comment on “setDirty( true )”