The Daily WTF: Curious Perversions in Information Technology

merreborn · 2006-11-29 Reply Admin

Anonymous:

Although why a language would need the Boolean class which is essentially a tri-state (true, false, null) rather than a native bool is beyond me.

Because for things like data structures, everything needs an object wrapper. For example, Hashtable.put() only takes objects as arguments.

2006-11-29 Reply Admin

bruzie:
Having done several contracts for different NZ Government departments, I can make a reasonable guess of the companies involved. It's probably one of these:
Inland Revenue/EDS
Accident Compensation Corporation/Unisys
My experiences when working at government department clients have always been consistent when dealing with their outsourced IT partners - they've been atrocious. Five days to get a log on, one day to get a password reset (what am I supposed to do in the meantime?). Unisys is worse - we had a Java enterprisey project and the Unisys Java Team Leader (Australian) was supposed to give us the EJB to work from - but went skiing instead, then was caught out in several lies trying to cover it up.
Grrrr.

I have to agree Accident Compensation Corporation/Unisys sounds like an excelent guess at this one. I have never had any dealings with Inland Revenue/EDS but can imagine they are not much better (or could be worse).

2006-11-29 Reply Admin

Anonymous:
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

Two problems.

1. You used "|| ((isRecordLocked = newValue).valueOf("false"))" -- a conditional OR operator. This means the assignment never takes place if the dirty flag is going to be set.

2. Even if you fix #1 by using a regular OR (single pipe '|' ), order of operations is changed -- setDirty() is called after the assignments, which may or may not affect the code. This is an iffy point.

Two lines :

for (int i = 0; (i++ == 0) && ((isRecordLocked == null)?(newValue != null):!isRecordLocked.equals(newValue)); context.setDirty(true));
isRecordLocked = newValue;

One line, ignoring order of operations:

for (int i = 0; ((i++ == 0) && ((isRecordLocked == null)?(newValue != null):!isRecordLocked.equals(newValue))) | ((isRecordLocked = newValue) != newValue); context.setDirty(true));

2006-11-29 Reply Admin

Anonymous:
This forum really is a haven for complete idiots. Only about three of the commenters actually understood what this code was doing (and I suspect the submitter didn't either). When they are going to so much effort to support tri-value booleans they probably have a reason for it. This code is slightly ugly but is not broken, unlike most of the suggested replacements.

So true, and tragicomically underlined by the fact that your own solution doesn't work either. Reading through all the responses brought me to the conclusion that 80% of the baboons that post to this forum wouldn't be able to peel a banana while blindfolded.

In fact, the few who're smarter than the rest have illustrated that (non-standard extension methods aside) there just isn't any way to accomplish what the cited function accomplishes in a true one-liner, unless you're going to make that line much longer than code readability guidelines recommend, flat out contradicting the submitter's claims. This, together with the correctness of the original code means The Real WTF is that this function ever made it to the "CodeSOD" at all. It is not perfect but far from a WTF. If this is the worst code in the entire project, I commend the developers for the excellent work.

Most of the suggested replacements, on the other hand, show that the submitter makes dangerous assumptions about the evaluated objects, or they are plain wrong even under the most favorable conditions. Maybe I'm too old school, but as someone who's allowed himself to be much more influenced by Donald "It works" Knuth than by Bill "It looks good and sells" Gates, I'm absolutely convinced that correct and slightly convoluted code is infinitely preferrable to elegant and incorrect solutions. Unfortunately, that once more seems to make me part of a minority.

2006-11-29 Reply Admin

Oh, and a third problem -- if newValue is null, you'd throw a NullPointerException

obediah · 2006-11-29 Reply Admin

Ottokar:
Anonymous:
Makes perfect sense to me, I cant see a third value in the Boolean object no matter how hard i look.
A simple example:
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.

And their was much gnashing of teeth and wrenching of hands and pounding of foreheads. And Obediah looked up and at his meakest spaketh,

"Oh, Great Lord - our crops whither and our people starve. We endeareth to make pleasure for thine eyes and spirit, but our booleans are sinful, broken creatures. All is well when they speak yay or nay, but their silence bringeth forth exception and behaviour most undefined."

And the Lord looked down on pitiful Obediah and his burning lands. He knew the people of earth were good and granted them a respite from this one trial - he granted them the gift of 'NOT NULL'. And there was much rejoicing and singing and sighs to acknowledging god's infinite wisdom. Except for some MySQL jerks.

VGR · 2006-11-29 Reply Admin

Anonymous:

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.

I agree. Every external dependency greatly reduces the portability and usability of one's code. Every extra jar an app needs is going to lower my enthusiasm for using the thing, and it doesn't take many before I decide it's just not worth the bother at all. (This also applies to many open source projects, both Java and non-Java.) It's another jar to have in the classpath, another API to become familiar with (if it even has javadoc), and some cases, the dependency on it is weaved into the code base and can never be removed without a sweeping rewrite.

So, knowing that an external dependency carries such a high cost, what is the benefit you get in exchange for taking such a hit? A class with an embarrassingly bad name ("ObjectUtils" is its own WTF, in my opinion), and a method that does something only slightly more complex than adding two numbers.

I would much rather put 'private static boolean equals(Object o1, Object o2) { return o1 == null ? (o2 == null) : o1.equals(o2); }' in multiple classes than be saddled with dependency on a third-party jar for something so trivial.

And I agree that many of Jakarta's offerings, particularly their Java offerings, are sloppy and unreliable. I like the web server, and I like ant, but that doesn't give the other projects a free pass. I have a strong dislike of the Jakarta Commons in particular.

2006-11-29 Reply Admin

obediah:
Ottokar:
Anonymous:
Makes perfect sense to me, I cant see a third value in the Boolean object no matter how hard i look.
A simple example:
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.

And their was much gnashing of teeth and wrenching of hands and pounding of foreheads. And Obediah looked up and at his meakest spaketh,
"Oh, Great Lord - our crops whither and our people starve. We endeareth to make pleasure for thine eyes and spirit, but our booleans are sinful, broken creatures. All is well when they speak yay or nay, but their silence bringeth forth exception and behaviour most undefined."
And the Lord looked down on pitiful Obediah and his burning lands. He knew the people of earth were good and granted them a respite from this one trial - he granted them the gift of 'NOT NULL'. And there was much rejoicing and singing and sighs to acknowledging god's infinite wisdom. Except for some MySQL jerks.

NOT NULL doesn't handle the problem, it skips them, which is different.

Here's an example: You have a Boolean field specifying an option. You can set it to Boolean.TRUE or Boolean.FALSE to explicitly set the option, or use null to indicate a default or inherited value.

FrostCat · 2006-11-29 Reply Admin

Anonymous:

Although why a language would need the Boolean class which is essentially a tri-state (true, false, null) rather than a native bool is beyond me.

All right, now stop that. Nullable variables are actually useful, as long as you understand what they're for.

FrostCat · 2006-11-29 Reply Admin

Anonymous:

Although why a language would need the Boolean class which is essentially a tri-state (true, false, null) rather than a native bool is beyond me.

All right, now stop that. Nullable variables are actually useful, as long as you understand what they're for.

obediah · 2006-11-29 Reply Admin

Anonymous:
NOT NULL doesn't handle the problem, it skips them, which is different.
Here's an example: You have a Boolean field specifying an option. You can set it to Boolean.TRUE or Boolean.FALSE to explicitly set the option, or use null to indicate a default or inherited value.

I wasn't attempting to speak to the Java Object issue, only the comment of null boolean support being needed in a programming language to handle null values in booleans taken from a database. NOT NULL nullifies that concern. :)

FWIW, I've never seen a situation where hacking a third state into a db boolean via NULL was the best option. I haven't seen all situations - but it's something I'd have to think about long and hard before implementing.

Thuktun · 2006-11-29 Reply Admin

Sheesh. A null reference in this case may indicating that a value has not yet been specified by anyone and needs to be specified before something else occurs.

The value is changed if (1) the reference is different, (2) becomes null, (3) becomes non-null, or (4) becomes something different. Leveraging the fact that Object.equals is supposed to return false when passed a null and omit (2), we can bundle checks (2) and (4) together.

public void setIsRecordLocked(Boolean newValue)
{
    // Update state if required.
    context.setDirty(
        isRecordLocked != newValue ||         // (1)
        isRecordLocked == null ||             // (3)
        !isRecordLocked.equals( newValue ) ); // (2) and (4)

    // Change the value.
    isRecordLocked = newValue;
}

The original quoted code is not incorrect, it's just overly verbose.

(The Real WTF (tm) is the HTML that this forum editor produces!)

2006-11-29 Reply Admin

Anonymous:
Anonymous wrote:
if (isRecordLocked != newValue)
No! In Java, Boolean is a class, not a simple type like bool, so:
The != operator tests whether the two object references refer to the same object; it does not check whether the two object references refer to objects with the same value.
Object references might be null, which you don't account for.
Someone has already beaten me to suggest the following replacement:
if (isRecordLocked==null || newValue==null || !isRecordLocked.equals(newValue))

if ((isRecordLocked != newValue) || isRecordLocked==null || newValue==null || !isRecordLocked.equals(newValue))

Adding in the direct comparison of the references handles the case of both being null.

aakoch · 2006-11-29 Reply Admin

Anonymous:
Oh, and a third problem -- if newValue is null, you'd throw a NullPointerException

No, it doesn't.

Here's my version:

/**    
    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 && newValue == null) {
        // do nothing
    }
    else if (isRecordLocked != null && newValue == null) {
        context.setDirty(true);
    }
    else if (isRecordLocked == null && newValue != null) {
        context.setDirty(true);
    }
    else if (!isRecordLocked.equals(newValue)) {
        context.setDirty(true);
    }
    // Change the value.
    isRecordLocked = newValue;
}

Clean and understandable. Any objections?

2006-11-29 Reply Admin

Thuktun:
Sheesh. A null reference in this case may indicating that a value has not yet been specified by anyone and needs to be specified before something else occurs.

The value is changed if (1) the reference is different, (2) becomes null, (3) becomes non-null, or (4) becomes something different. Leveraging the fact that Object.equals is supposed to return false when passed a null and omit (2), we can bundle checks (2) and (4) together.
public void setIsRecordLocked(Boolean newValue)
{
    // Update state if required.
    context.setDirty(
        isRecordLocked != newValue ||         // (1)
        isRecordLocked == null ||             // (3)
        !isRecordLocked.equals( newValue ) ); // (2) and (4)

    // Change the value.
    isRecordLocked = newValue;
}
The original quoted code is not incorrect, it's just overly verbose.

(The Real WTF (tm) is the HTML that this forum editor produces!)

As has been pointed out before, calling setDirty(false) is WRONG. Practical reason why? Because if you call setIsRecordLocked twice in a row with the same value, the dirty flags is set to true, and then false, which is very likely wrong.

2006-11-29 Reply Admin

aakoch:
Anonymous:
Oh, and a third problem -- if newValue is null, you'd throw a NullPointerException

No, it doesn't.

Yes it does. You missed this part:

if(
            (isRecordLocked != null && !isRecordLocked.equals(newValue)) ||
            (newValue != null && !newValue.equals(isRecordLocked)) ||
            ((isRecordLocked = newValue).valueOf("false"))
            )

If newValue is null, isRecordLocked is set to null, and then .valueOf() is called on it.

aakoch · 2006-11-29 Reply Admin

Thuktun:
Sheesh. A null reference in this case may indicating that a value has not yet been specified by anyone and needs to be specified before something else occurs.

The value is changed if (1) the reference is different, (2) becomes null, (3) becomes non-null, or (4) becomes something different. Leveraging the fact that Object.equals is supposed to return false when passed a null and omit (2), we can bundle checks (2) and (4) together.
public void setIsRecordLocked(Boolean newValue)
{
    // Update state if required.
    context.setDirty(
        isRecordLocked != newValue ||         // (1)
        isRecordLocked == null ||             // (3)
        !isRecordLocked.equals( newValue ) ); // (2) and (4)

    // Change the value.
    isRecordLocked = newValue;
}
The original quoted code is not incorrect, it's just overly verbose.

(The Real WTF (tm) is the HTML that this forum editor produces!)

This doesn't work. Try running it.

Also, you don't want to set dirty to false if somewhere else already set it to true.

aakoch · 2006-11-29 Reply Admin

Anonymous:
aakoch:
Anonymous:
Oh, and a third problem -- if newValue is null, you'd throw a NullPointerException

No, it doesn't.
Yes it does. You missed this part:
if(
            (isRecordLocked != null && !isRecordLocked.equals(newValue)) ||
            (newValue != null && !newValue.equals(isRecordLocked)) ||
            ((isRecordLocked = newValue).valueOf("false"))
            )
If newValue is null, isRecordLocked is set to null, and then .valueOf() is called on it.

Sorry. Thought you were talking about the original post. I wasn't following the discussion closely enough. My apologies.

2006-11-29 Reply Admin

Anonymous:
aakoch:
Anonymous:
Oh, and a third problem -- if newValue is null, you'd throw a NullPointerException

No, it doesn't.
Yes it does. You missed this part:
if(
            (isRecordLocked != null && !isRecordLocked.equals(newValue)) ||
            (newValue != null && !newValue.equals(isRecordLocked)) ||
            ((isRecordLocked = newValue).valueOf("false"))
            )
If newValue is null, isRecordLocked is set to null, and then .valueOf() is called on it.

Oops, you're right - valueOf() is a static method. Bad form, but valid Java.

mrprogguy · 2006-11-29 Reply Admin

Anonymous:

Anonymous wrote:
if (isRecordLocked != newValue)
No! In Java, Boolean is a class, not a simple type like bool, so:
The != operator tests whether the two object references refer to the same object; it does not check whether the two object references refer to objects with the same value.
Object references might be null, which you don't account for.
Someone has already beaten me to suggest the following replacement:
if (isRecordLocked==null || newValue==null || !isRecordLocked.equals(newValue))

Just another in a long, long, list of reasons not to use Java. Ever. For anything.

aakoch · 2006-11-29 Reply Admin

mrprogguy:
Just another in a long, long, list of reasons not to use Java. Ever. For anything.

You can use "boolean" if you like. It's not a class.

2006-11-29 Reply Admin

Anonymous:
This forum really is a haven for complete idiots. Only about three of the commenters actually understood what this code was doing (and I suspect the submitter didn't either). When they are going to so much effort to support tri-value booleans they probably have a reason for it. This code is slightly ugly but is not broken, unlike most of the suggested replacements.
For anyone using Java who needs to compare possibly null objects may I suggest Jakarta commons-lang which include the Object.equals methods in a previous comment as ObjectUtils.equals (along which a bunch of other useful stuff which probably should be in Java - and some complete crap of course).

My solution would be to change the Context class:
class Context {
public void setDirtyIfDifferent(Object obj1, Object obj2) {
setDirty(!ObjectUtils.equals(obj1, obj2));
}
}
then the original method could be:
public void setIsRecordLocked(Boolean newValue) {
    context.setDirtyIfDifferent(newValue, isRecordLocked);

    // Change the value.
    isRecordLocked = newValue;
}

Could not agree more. Never before have I seen so many totally incompetent people gathering together over a piece of code, trying to be "clever", not having the slightest idea of what they are talking about.

obediah · 2006-11-29 Reply Admin

Anonymous:

Could not agree more. Never before have I seen so many totally incompetent people gathering together over a piece of code, trying to be "clever", not having the slightest idea of what they are talking about.

You must not have a CS degree.

2006-11-29 Reply Admin

This is actually longer but may be more clear (or not):

return (isRecordLocked==null) != (newValue==null)
       || 
       isRecordLocked!=null   <font color="green">// prevent null pointer exception</font>
       && !isRecordLocked.equals(newValue);

Or here's another way:

Object o1 = (isRecordLocked==null ? "null" : isRecordLocked);
Object o2 = (newValue==null ? "null" : newValue);
return !o1.equals(o2);

2006-11-29 Reply Admin

Anonymous:
Anonymous:
This forum really is a haven for complete idiots. Only about three of the commenters actually understood what this code was doing (and I suspect the submitter didn't either). When they are going to so much effort to support tri-value booleans they probably have a reason for it. This code is slightly ugly but is not broken, unlike most of the suggested replacements.

So true, and tragicomically underlined by the fact that your own solution doesn't work either. Reading through all the responses brought me to the conclusion that 80% of the baboons that post to this forum wouldn't be able to peel a banana while blindfolded.

In fact, the few who're smarter than the rest have illustrated that (non-standard extension methods aside) there just isn't any way to accomplish what the cited function accomplishes in a true one-liner, unless you're going to make that line much longer than code readability guidelines recommend, flat out contradicting the submitter's claims. This, together with the correctness of the original code means The Real WTF is that this function ever made it to the "CodeSOD" at all. It is not perfect but far from a WTF. If this is the worst code in the entire project, I commend the developers for the excellent work.

Most of the suggested replacements, on the other hand, show that the submitter makes dangerous assumptions about the evaluated objects, or they are plain wrong even under the most favorable conditions. Maybe I'm too old school, but as someone who's allowed himself to be much more influenced by Donald "It works" Knuth than by Bill "It looks good and sells" Gates, I'm absolutely convinced that correct and slightly convoluted code is infinitely preferrable to elegant and incorrect solutions. Unfortunately, that once more seems to make me part of a minority.

I have to agree here. It took the original coder two minutes to write the code; it takes a decent coder two minutes to understand the code. And it works. There's nothing complicated about it. Instead of wasting time trying to optimize this code to some optimal one liner, I hope the original programmer spent his time working on more substantial things.

emurphy · 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.

It's still non-trivial to understand, but once you do understand it, it's actually elegant.

VGR:
private static boolean equals(Object o1, Object o2) { return o1 == null ? (o2 == null) : o1.equals(o2); }

This almost abstracts the elegance of the above. I would prefer

private static boolean equals(Object o1, Object o2) { return o1 == null ? (o1 == o2) : o1.equals(o2); }

2006-11-29 Reply Admin

OK, I'll hold myself up for ridicule. I wrote out the truth table: 9 entries, with newValue and isRecordLocked being 0, 1 or NULL. "context.SetDirty (true)" is called everywhere except the middle diagonal. If both inputs are null, or if the inputs have equal value, then "context.SetDirty(true)" is not called.

My suggestion would be split off the no-op cases and judiciously comment:

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

return; // take no action if both are null

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

return; // take no action if both inputs are non-null and are equal

// fall-through: variables differ, so set the dirty flag and update isRecordLocked

context.setDirty(true);

isRecordLocked = newValue;

Granted, this differs from the example code itself because it only updates isRecordLocked when required, but it does follow the comment in the original code.

I don't see how a one-liner would be warranted here since equals can't be called on a null source or target. Caveat: I don't know Java, only C/C++.

2006-11-29 Reply Admin

Anonymous:

OK, I'll hold myself up for ridicule. I wrote out the truth table: 9 entries, with newValue and isRecordLocked being 0, 1 or NULL. "context.SetDirty (true)" is called everywhere except the middle diagonal. If both inputs are null, or if the inputs have equal value, then "context.SetDirty(true)" is not called.
My suggestion would be split off the no-op cases and judiciously comment:
if (newValue == null && isRecordLocked == null)
return; // take no action if both are null
if (newValue != null && isRecordLocked != null && isRecordLocked.equals (newValue))
return; // take no action if both inputs are non-null and are equal
// fall-through: variables differ, so set the dirty flag and update isRecordLocked
context.setDirty(true);
isRecordLocked = newValue;

Granted, this differs from the example code itself because it only updates isRecordLocked when required, but it does follow the comment in the original code.
I don't see how a one-liner would be warranted here since equals can't be called on a null source or target. Caveat: I don't know Java, only C/C++.

Close, but there is a difference -- whether it is significant can't be inferred from the code snippet. The condition "if (newValue != null && isRecordLocked != null && isRecordLocked.equals (newValue))" should set "isRecordLocked = newValue;" before returning, because although they may be .equals() (equal logical value), they may not be == (same reference).

emurphy · 2006-11-29 Reply Admin

Anonymous:
I don't see how a one-liner would be warranted here since equals can't be called on a null source or target. Caveat: I don't know Java, only C/C++.

equals() can't be called on a null source, but you can pass a null target to it.

newfweiler · 2006-11-29 Reply Admin

Am I the frist* to notice that this code is not thread-safe?

Suppose a maintenance thread is triggered by setDirty(true) to write out the changed data. (For that matter, suppose it's a single-threaded application and setDirty(true) writes out the changed data and then returns.) Dirty is then set false. The assignment to isRecordLocked could happen after that.

*Frist: An expert who makes a diagnosis based on scanty data. After Dr. Bill Frist, United States Senator (R., Tennessee) who diagnosed Terry Sciavo by watching a video.

2006-11-29 Reply Admin

newfweiler:

Am I the frist* to notice that this code is not thread-safe?
Suppose a maintenance thread is triggered by setDirty(true) to write out the changed data. (For that matter, suppose it's a single-threaded application and setDirty(true) writes out the changed data and then returns.) Dirty is then set false. The assignment to isRecordLocked could happen after that.

For all we know, the synchronization/locking could be done outside of the code. It's beyond the scope of the snippet.

2006-11-29 Reply Admin

Anonymous:

OK, I'll hold myself up for ridicule. I wrote out the truth table: 9 entries, with newValue and isRecordLocked being 0, 1 or NULL. "context.SetDirty (true)" is called everywhere except the middle diagonal. If both inputs are null, or if the inputs have equal value, then "context.SetDirty(true)" is not called.

My suggestion would be split off the no-op cases and judiciously comment:

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

return; // take no action if both are null

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

return; // take no action if both inputs are non-null and are equal

// fall-through: variables differ, so set the dirty flag and update isRecordLocked

context.setDirty(true);

isRecordLocked = newValue;

Granted, this differs from the example code itself because it only updates isRecordLocked when required, but it does follow the comment in the original code.

I don't see how a one-liner would be warranted here since equals can't be called on a null source or target. Caveat: I don't know Java, only C/C++.

I think we can step it up a bit.

You want to return if either condition happens, so:

if ((newValue == null && isRecordLocked == null) ||
(newValue != null && isRecordLocked != null && isRecordLocked.equals (newValue)))
return;

That's the first step we can perform. Next, we are effectively calling setDirty if all of this is not true. So we can instead do this:

if (!((newValue == null && isRecordLocked == null) ||
(newValue != null && isRecordLocked != null && isRecordLocked.equals (newValue))))
setDirty(true);

isRecordLocked = newValue is always performed, regardless of the value of newValue. The comments bear this out. It says that the isRecordLocked is changed to the value of newValue without imposing any conditions on the assignment. The only conditional is to whether or not to mark it "dirty". And basically, it only wants to mark it "dirty" if the value of isRecordLocked changes. And even still, I don't believe assignment to the same value is going to make that much of a difference, so in or out of the if block is largely semantic as assignment should be trivial. (Unless, we are assigning the whole newValue object to isRecordLocked, in which case, I'd say always perform the assignment as the two objects are probably always different and it may affect something else if it doesn't happen. (You can tell I have the whole, "Don't change anything you have the slightest doubt over" mentality)).

I believe a cleaner implementation would have it always mark "dirty". Unless the "cleansing code" is a bottleneck and this whole "dirty marker" was used to create conditional execution. In that case, there is probably a better way to solve the performance issue than to code around the problem.

2006-11-29 Reply Admin

Anonymous:
bruzie:
Having done several contracts for different NZ Government departments, I can make a reasonable guess of the companies involved. It's probably one of these:
Inland Revenue/EDS
Accident Compensation Corporation/Unisys
My experiences when working at government department clients have always been consistent when dealing with their outsourced IT partners - they've been atrocious. Five days to get a log on, one day to get a password reset (what am I supposed to do in the meantime?). Unisys is worse - we had a Java enterprisey project and the Unisys Java Team Leader (Australian) was supposed to give us the EJB to work from - but went skiing instead, then was caught out in several lies trying to cover it up.
Grrrr.

I have to agree Accident Compensation Corporation/Unisys sounds like an excelent guess at this one. I have never had any dealings with Inland Revenue/EDS but can imagine they are not much better (or could be worse).

I'm going to guess at Ministry of Economic Development/Hewlett Packard.

www.companies.govt.nz - Witness the amazing fusion of Plone and J2EE.

2006-11-29 Reply Admin

Well said :-).

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.

Finally, a sane replacement that actually works! I would think that

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

would be clearer, but that's just me.

2006-11-29 Reply Admin

Wiggles:
Goplat:

If you compare Boolean (or any reference type, actually) with != you are comparing the references, not the object contents like the original function does.
This makes the assumption that recordLocked is a Boolean. If it's of the primitive type boolean and Java 1.5 is being used then autoboxing will come into effect and simple object equality == would work fine.

In Java there are just two possible Boolean's, one wraps true and the other wraps false. Yes, you are comparing references but since there are only two == and != work fine.

chrismcb · 2006-11-29 Reply Admin

Anonymous:

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

Yeah I generally write A == B all the time when I really mean B == null, because A == B makes my intentions that I am comparing B to NULL very clear. Clear as in clear as mud.

You know I realize what you are saying. If you have "if B == null then if B == A...", then its the same as "if B == null then if null == A..." But its not CLEARER, its muddier. When you are reading the statement B == A you have to realize you are in the portion where B == null case. WHY make the reader work harder when you can just explictly spell it out?

chrismcb · 2006-11-29 Reply Admin

I'm amazed (although I guess I shouldn't be.) I assumed that most of the readers here were above some of the nonsense that shows up on thedailwtf, instead I see many of the readers here are boning up on trying to write their own contributions.

Some people claim this isn't a WTF, and its simple, yet there are almost 200 comments about it.

Some people write "clearer" code that is more difficult to read than the original.

Some people don't understand that the original isn't wrong, it just isn't clear.

Most people can't seem to understand how to write this correctly (even if the code is messier than the original)

A bunch of people offered "clearer" code that sets the dirty flag more times than the original does. And some point out that there is nothing wrong with the original. You do realize that you should never have the exact same code in two different part of the conditionals (except in extreme cases) Simplify/rewrite the conditional to lead to the same statement. In this particular case the line of code being executed was a simple assignment. But what if you needed to add a second statement. Now you have to do it in two places (or 4 in one "clearer" example) if there is a bug you have to fix it in two places (or probably fix it in one, forget or not know to fix it in the other) You may argue that this is a simple case and its ok to break the rules. Well you shouldn't. You should also strive to write good code. Break the rules only for extreme exceptions.

There were over 180 posts, and there was only ONE solution that looked simpler and was correct. (There were a few that were correct but not as simple as could be.) Why do "bool b = blah; if b then do blah?" When you can just say "if blah then do blah?"

My only question, is it not valid to say:

foo = null

if someboolean.equals(foo)???? If you can't do this, then java is really lame.

2006-11-29 Reply Admin

Anonymous:
Anonymous:
Anonymous wrote:
if (isRecordLocked != newValue)
No! In Java, Boolean is a class, not a simple type like bool, so:
The != operator tests whether the two object references refer to the same object; it does not check whether the two object references refer to objects with the same value.
Object references might be null, which you don't account for.
Someone has already beaten me to suggest the following replacement:
if (isRecordLocked==null || newValue==null || !isRecordLocked.equals(newValue))

if ((isRecordLocked != newValue) || isRecordLocked==null || newValue==null || !isRecordLocked.equals(newValue))
Adding in the direct comparison of the references handles the case of both being null.

NO, NO, NO! In Java you can directly compare Boolean's and get the same result as if you unboxed both variables. Check out http://java.sun.com/docs/books/jls/third_edition/html/expressions.html#15.21.2

2006-11-29 Reply Admin

I personally agree with you, but the reason for B==A is emphasize the parallel to B.equals(A).

VGR · 2006-11-29 Reply Admin

Anonymous:
In Java there are just two possible Boolean's, one wraps true and the other wraps false. Yes, you are comparing references but since there are only two == and != work fine.

Not correct. 'new Boolean(true)' will always create a new object. It is not the same object as Boolean.TRUE. Comparing it to Boolean.TRUE using the '==' operator will return false. Try it for yourself.

Anonymous:
NO, NO, NO! In Java you can directly compare Boolean's and get the same result as if you unboxed both variables. Check out http://java.sun.com/docs/books/jls/third_edition/html/expressions.html#15.21.2

You may want to read that link more closely. It states, more than once, that unboxing is done if ONE of the operands is a Boolean instance. The very next section states that if both operands are reference types (that is, objects), the operation is reference equality.

2006-11-29 Reply Admin

Why is this a WTF - it's a property accessor method for the isRecordLocked field which is a Boolean object (which in this particular language can be NULL). When the value is changed, the isDirty field also needs to be set. It's all pretty standard stuff - maybe it could be refactored a little, and it certainly looks like it was generated using some sort of template or code generation system - but at the end of the day it would be WTF if all the accessor didn't check for NULL before using the object - that's far more common.

Incidentally I've worked for a large NZ Govt transmission company - they had real WTF code - a Fortran code (8000 lines all in 1 function) which have a different variable for every property of every powerstation in New Zealand... Compiler used to fall over presumably

2006-11-29 Reply Admin

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

Yeah, it works that way in SQL, but not in Java.

If this were my code, I'd be grepping through the source to see if I could change to 'boolean's instead of 'Boolean's, which is my preferred way to simplify this method.

2006-11-30 Reply Admin

OMG WTF at the comments!

The original code is correct and about as readable as any proposed alternative, and way more correct than most (go back to VB you wankers!). The WTF with it is that setIsRecordLocked should have been defined to take a boolean not Boolean.

DUUUUUUH!

2006-11-30 Reply Admin

people...

dirty = newValue != isRecordLocked

2006-11-30 Reply Admin

oops. forgot this was java code... once again, I say people...

dirty = !((newValue == isRecordLocked)||(newValue!=null && newValue.equals(isRecordLocked)));

2006-11-30 Reply Admin

The real WTF is the undocumented side-effect.

This method would also definitely benefit from a couple of unit tests - which, of course, may well exist.

DagSverre · 2006-11-30 Reply Admin

> oops. forgot this was java code... once again, I say people...
> dirty = !((newValue == isRecordLocked)||(newValue!=null && newValue.equals(isRecordLocked)));

No no no. Don't try to be clever. Here's why:

(original state of o: not dirty, not locked)

o.setRecordLocked(true);
o.setRecordLocked(true);

Result: o is NOT dirty, but has changed state to locked. If you want a "nice oneliner" syntax then dirty = dirty && ... will do it, but rewriting if-conditions to boolean operator logic is hardly something that is very important.

Can we all stop discussing a perfectly good piece of code now? Just because it is posted on TDWTF apparently doesn't mean there's something wrong with it.

Earl Purple · 2006-11-30 Reply Admin

No, I would have to say that the real wtf is using Boolean instead of an enum that can have 3 possible values, although Java has only just introduced enums in version 5.

public enum LockStatus { UNKNOWN, UNLOCKED, LOCKED };
private LockStatus lockStatus;

public void setLockStatus( LockStatus newValue )
{
    if ( newValue != lockStatus )
    {
        context.setDirty( true );
        lockStatus = newValue;
    }
}

Now doesn't that make life a whole lot simpler?

emurphy · 2006-11-30 Reply Admin

Earl Purple:
No, I would have to say that the real wtf is using Boolean instead of an enum that can have 3 possible values, although Java has only just introduced enums in version 5.
public enum LockStatus { UNKNOWN, UNLOCKED, LOCKED };
private LockStatus lockStatus;
public void setLockStatus( LockStatus newValue )
{
    if ( newValue != lockStatus )
    {
        context.setDirty( true );
        lockStatus = newValue;
    }
}
Now doesn't that make life a whole lot simpler?

Yes, it doesn't. Wait till your cow orkers get a hold of this, and insist on adding MAYBELOCKED and VERYLOCKED and FILENOTFOUND.

Won't someone please think of the children?

setDirty( true )

Leave a comment on “setDirty( true )”