- Feature Articles
- CodeSOD
- Error'd
- Forums
-
Other Articles
- Random Article
- Other Series
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
Actually, having them pass an enum can be more descriptive than getting them to pass in true or false.
Have a look at a function call made to an API that passes parameters as true or false etc and see if you can tell without looking up in the API what those boolean values stand for.
Now instead if you pass in LOCKED or UNLOCKED it can make a lot more sense.
And it's easier to add more enums if necessary. How would you do that with Boolean?
And how is MAYBELOCKED different from UNKNOWN? I was just showing how to write a tri-state properly.
Admin
public void setIsRecordLocked(Boolean newValue) {
And look up short-circuiting before telling me it'll throw an NPE.if (isRecordLocked!=newValue && (isRecordLocked==null || newValue==null || !isRecordLocked.equals(newValue)) {
context.setDirty(true);
}
isRecordLocked = newValue;
}
TRWTF are the comments.
Admin
This is fair enough. I usually use the jakarta commons stuff because I already am stuck with it as it is a dependency of some other library I am using :)
(Though I think HttpClient is pretty good, and collections and logging just should not exist)
Admin
It isn't, really. The original quoted version appears to be correct. All the flailing about for replacements that don't quite work correctly would seem to indicate that it's not as simple as everyone seems to think at first glance.
Admin
I also don't see why a record lock should be a tri-state situation. Either it's locked or it isn't.
The initial state should be unlocked until someone locks it.
Admin
Nope. You have one more guess what the correct operator is.
Captcha: genius (the opposite of a TDWTF comment poster)
Admin
Another kick at the cat.
The 1-liner solution is a trick. The much more understandable, intention-revealing 1-liner that preserves the original code is:
SetDirtyIfDifferent( isRecordLocked, newValue );
To me the WTF is that it's hard to tell it's just a property-setter that flags a dirty bit (or whatever SetDirty() does) when the value changes. I'd bet the whole class (and hierarchy) is littered with that code copy-pasted. Wheeeeee:
1. You're micromanaging the dirty state of the object in each and every property setter and probably reusing the same code all over the place - write the code once.
2. Do you really want to SetDirty(false)? Isn't it more intention-revealing to have SetDirty() and ClearDirty()?
(BTW I swear that I wrote this before reading all of replies and only before posting noticed Asd's response and LOL'd when I realised we both called it SetDirtyIfDifferent :-)
The 1-liner bit is a trick. Trying to shove all of that into a single line is unreadable and bad practice. Some of the solutions are more of a wtf than the original. Truly shocking.
As others point out, Tri-state booleans are legal and necessary. Think of it as Yes / No / Don't know. That said, using null is probably a bad choice for this. An enum would have been better. For more complicated objects, using the Null Object pattern might make more sense so client code isn't continually comparing against null.
Admin
Admin
Ouch! That should teach me something about commenting the mistakes of others :-)
Admin
That works, but can be simplified to this, which someone has already posted (or maybe it was something very similar):
public void setIsRecordLocked(Boolean newValue) {
if (isRecordLocked!=newValue && (isRecordLocked==null || !isRecordLocked.equals(newValue)) {
context.setDirty(true);
}
isRecordLocked = newValue;
}
Admin
It would need to be dirty = dirty || ...
or preferably dirty ||= ...
but "context" seems to be an external object so that is not possible.
There is something strange about the name of the function though. Are we marking the record as dirty if it has changed state so that we know we must update it when we come to persist it? Why are we calling setIsRecordLocked, why not lock() or unlock(). And locking a record by itself does not make it "dirty".
I would like to know the context that this code is in.
Admin
And therein lies the problem. Its only a "parallel." The whole concept of "left as an exercise to the reader" should not apply to code. Give the reader (read future maintainers) as much information as possible. Make it as easy as possible to understand what is going on. You are trying to write the function "B.equals(A)" but you can't if B is null. Be concise, and explicit. if you are comparing a value to null, compare it to null. Don't compare it to something that is sometimes null. It would be a different story if you had some constant that equaled null.
For those readers who missed the original quote. It was suggested to write:
"if (B == null) then if (B == A) ..." instead of "if (B == null) then if (null == A) ..." as the first is "clearer," which of course it isn't.
Here is the problem. You can't simply write "if B == A" if both aren't NULL. If someone who is in a hurry, and doesn't bother to read the entire statement sees this holy trinity of if's, they might think "Well geeze we are trying to compare B and A, so lets just delete the null check, and leave if (B == A)" At first glance it seems the right thing to do, but as many people already pointed out, you can't do that.
So you just made it harder for future maintainers to maintain this code, just so you can keep your little "parallel" going. If you mean null then say it.
Admin
public bool compare(Object o1, Object o2) {
return (o1 == null || o2 == null) ? o1 == o2 : o1.equals(o2);
}
public void setIsRecordLocked(Boolean newValue) {
if (!compare(isRecordLocked, newValue)) context.setDirty(true);
isRecordLocked = newValue;
}
Admin
In the case of "setIsRecordLocked(Boolean newValue)", it's obvious what the boolean values stand for. That is not the WTF.
The intent is to compare A and B. Whether they are null or not-null, the intent is still to compare A to B. Placing "A == null" (or "null == B", etc.) after the ? part of the operator obscures this purpose.
In either case, the proper way to prevent future coders in a hurry from improperly collapsing the logic is to comment it:
boolean AreEqual(Object a, Object b) {
return (a != null)
? (a.Equals(b)) // use Equals() if possible
: (a == b); // otherwise use ==
}
or
boolean AreEqual(Object a, Object b) {
return (a == null)
? (b == null) // if a is null, then only another null can be equal to it
: (a.Equals(b)); // if a is not null, then use a.Equals()
}
Admin
The way I would have done this is by doing:
public void setIsRecordLocked(Boolean newValue) { // Update state if required. context.setDirty(!(isRecordLocked == newValue)); isRecordLocked = newValue; // Change the value.
}
but this is two lines.
Admin
Admin
Nope, the API documentation says this: "The equals method implements an equivalence relation on non-null object references". If newValue is null and isRecordLocked isn't, then the results of your method are undefined. Probably it'll throw an NPE!
I think it was posted as
setDirty(isRecordLocked!=newValue && (isRecordLocked==null || !isRecordLocked.equals(newValue))
, which is just wrong.Admin
Blast! Just re-read the documentation. It also says that x.equals(null) must always return false. So you win.
Captcha = truthiness, at last.
Admin
Admin
Don't read into the semantics of what the property of IsRecordLocked really is. You're just making an assumption: read it as setIsFoo.
Don't read into the semantics that it's a java boolean and forget your knowledge of that. So what, it's a class, and the reference can be null. I'm glad you understand object.Equals semantics, it's important, but it does require the object to be non-null. Some languages (like c#) have overrides so that you can again write o1 == o2. Java might be 'correct' in comparing references, but it sure does confuse a lot of people, but that's not the point either. Figure it out, it's your job.
Here I'll randomly take some posters simplification. I'm not checking it for correctness. I don't care for my illustration if it's deficient in some way.
<font face="Courier New">public void setFoo(Boolean newValue) {
if (isFoo!=newValue && (isFoo==null || newValue==null || !isFoo.equals(newValue)) {
context.setDirty(true);
}
isFoo= newValue;
}
<font face="Courier New">public void setIsBar(Boolean newValue) {
if (isBar!=newValue && (isBar==null || newValue==null || !isBar.equals(newValue)) {
context.setDirty(true);
}
isBar= newValue;
}</font>
<font face="Courier New">public void setIsBaz(Boolean newValue) {
if (isBaz!=newValue && (isBaz==null || newValue==null || !isBar.equals(newValue)) {
context.setDirty(true);
}
isBaz= newValue;
}</font>
// VERSUS
<font face="Courier New">public void setFoo(Boolean newValue) {</font><font face="Courier New">
SetDirtyIfDifferent(isFoo, newValue);
isFoo= newValue;
}
<font face="Courier New">public void setIsBar(Boolean newValue) {
SetDirtyIfDifferent(isBar, newValue);
isBar= newValue;
}</font>
<font face="Courier New">public void setIsBaz(Boolean newValue) {
SetDirtyIfDifferent(isBaz, newValue);
isBaz= newValue;
}</font>
// You could even make this easier yet by having SetDirtyIfDifferent return newValue, but that's just icing.
public void setFoo(Boolean newValue) {<font face="Courier New">
isFoo = SetDirtyIfDifferent(isFoo, newValue);
}</font>
PS In the original setIsBaz() did you spot the (intentional) copy-paste error? That's a subtle bug just waiting to happen. That's why you write the code just once to eliminate subtle bugs and to better communicate intention. I don't even need to pepper comments around because the method name is self-explanitory.
</font><font face="Courier New">public void setIsBar(Boolean newValue) {
SetDirtyIfDifferent(isBar, newValue);
isBar= newValue;
}</font>
<font face="Courier New">public void setIsBaz(Boolean newValue) {
SetDirtyIfDifferent(isBaz, newValue);
isBaz= newValue;
}</font>
// You could even make this easier yet by having SetDirtyIfDifferent return newValue, but that's just icing.
public void setFoo(Boolean newValue) {<font face="Courier New">
isFoo = SetDirtyIfDifferent(isFoo, newValue);
}</font>
PS In the original setIsBaz() did you spot the (intentional) copy-paste error? That's a subtle bug just waiting to happen. That's why you write the code just once to eliminate subtle bugs and to better communicate intention. I don't even need to pepper comments around because the method name is self-explanitory.
</font><font face="Courier New">public void setIsBar(Boolean newValue) {
if (isBar!=newValue && (isBar==null || newValue==null || !isBar.equals(newValue)) {
context.setDirty(true);
}
isBar= newValue;
}</font>
<font face="Courier New">public void setIsBaz(Boolean newValue) {
if (isBaz!=newValue && (isBaz==null || newValue==null || !isBar.equals(newValue)) {
context.setDirty(true);
}
isBaz= newValue;
}</font>
// VERSUS
<font face="Courier New">public void setFoo(Boolean newValue) {</font><font face="Courier New">
SetDirtyIfDifferent(isFoo, newValue);
isFoo= newValue;
}
<font face="Courier New">public void setIsBar(Boolean newValue) {
SetDirtyIfDifferent(isBar, newValue);
isBar= newValue;
}</font>
<font face="Courier New">public void setIsBaz(Boolean newValue) {
SetDirtyIfDifferent(isBaz, newValue);
isBaz= newValue;
}</font>
// You could even make this easier yet by having SetDirtyIfDifferent return newValue, but that's just icing.
public void setFoo(Boolean newValue) {<font face="Courier New">
isFoo = SetDirtyIfDifferent(isFoo, newValue);
}</font>
PS In the original setIsBaz() did you spot the (intentional) copy-paste error? That's a subtle bug just waiting to happen. That's why you write the code just once to eliminate subtle bugs and to better communicate intention. I don't even need to pepper comments around because the method name is self-explanitory.
</font><font face="Courier New">public void setIsBar(Boolean newValue) {
SetDirtyIfDifferent(isBar, newValue);
isBar= newValue;
}</font>
<font face="Courier New">public void setIsBaz(Boolean newValue) {
SetDirtyIfDifferent(isBaz, newValue);
isBaz= newValue;
}</font>
// You could even make this easier yet by having SetDirtyIfDifferent return newValue, but that's just icing.
public void setFoo(Boolean newValue) {<font face="Courier New">
isFoo = SetDirtyIfDifferent(isFoo, newValue);
}</font>
PS In the original setIsBaz() did you spot the (intentional) copy-paste error? That's a subtle bug just waiting to happen. That's why you write the code just once to eliminate subtle bugs and to better communicate intention. I don't even need to pepper comments around because the method name is self-explanitory.
Admin
Whoops .. what happened there? PEBKAC
The whole design looks like it's compensating for indescriminate setting of properties by client code. Fix the client instead.
void setIsRecordLocked( Boolean newValue )
{
SetDirty();
isRecordLocked = newValue;
}
Admin
You are supposed to call rs.wasNull() to determine if the last value read from the recordset was null.
Thus, the tristate Boolean can be handled by the code.
Admin
Just curious... after several pages of excellent boolean arguments, why would it matter if "isRecordLocked" was dirty or not?
Admin
After reading 4 pages of comments, I have to disagree. This is the best WTF I've seen in a long time.
Admin
Solution on Reddit
Admin
Ooops...
This solution will assign False to context.SetDirty where the original code would only assign True. The side effect of this code that is not present in the original would be to clear a True SetDirty from a prior execution.
Admin
don't u mean?:
public void setIsRecordLocked(Boolean newValue) {
if (isRecordLocked != newValue)
{
context.setDirty(true);
} else {
isRecordLocked = newValue;
}
}
Admin
LMFAO
Admin
You are right, it does not call setDirty if both newValue and isRecordLocked hold the same value (true ot false).
My fault!!
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()?)
Finally, we pointless set a value which is already false to false.
Admin
In Java objects can be null so you should already have one of these:
Then you can rewrite the original method like so:
You will probably find many times for reusing this utility method and improves readability a great deal.
Admin
How about this?
if(newValue!=NULL) { if (isRecordLock==NULL or !isRecordLock.Equals(newValue)) { context.SetDirty(True) isRecordLock=newValue; } }
Admin
if (!(isRec == newVal == null || isRec == newVal != null)) setDirty(true);
isRec = newVal;
Admin
OMG. I think I wrote that code. At least, I did write code for large NZ Govt departments a while ago, and it looks horribly familiar.
If so:
Admin
The TWTF is 4 pages of comments (and maybe even the original post) that nearly all misunderstood the logic of the nested ifs - and even the commenters that fully understood what these did, failed to see that it's just an exclusive or. (A "^" operator in c, and according to Wikipedia, also in Java and many other languages.) Do computer science or programming majors even learn Boolean logic anymore?
Ignore the record locking, tristate Boolean data, and the final line that unconditionally updates isRecordLocked. The form is:
if (A) { if (!B) C; } else { if(B) C; }
In any language with an exclusive-or operator, this simplifies to:
if (A ^ B) C;
That is, all the nested ifs reduce to one line:
if ((isRecordLocked != null) ^ (isNewValue != null)) { context.setDirty(true); }