• (cs) in reply to emurphy

    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.

     

  • Anonymous (unregistered)

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

    And look up short-circuiting before telling me it'll throw an NPE.

    TRWTF are the comments.

  • Asd (unregistered) in reply to VGR
    VGR:

    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.

    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)

  • (cs) in reply to Why?

    Anonymous:
    Why is this a WTF

    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.

  • (cs) in reply to Thuktun

    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.

     

  • AdT (unregistered) in reply to DagSverre
    DagSverre:

    If you want a "nice oneliner" syntax then dirty = dirty && ... will do it

    Nope. You have one more guess what the correct operator is.

    Captcha: genius (the opposite of a TDWTF comment poster)

  • Web 1.0 (unregistered) in reply to AdT

    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()?

    // assumption: value == null is a valid value
    

    public void SetMyValue( Boolean value ) { SetDirtyIfDifferent( myValue, value ); myValue = value; }

    // this can probably be pulled up to a common base class. private void SetDirtyIfDifferent( Boolean currentValue, Boolean newValue ) { // both values null if( currentValue == null && newValue == null ) return;

    // one of the values is null implies dirty
    if( (currentValue == null && newValue != null) ||
        (currentValue != null && newValue == null) )
    {
    	SetDirty();
    	return;
    }
    
    // nether value is null, dirty if different
    if( !currentValue.Equals(newValue) )
    {
    	SetDirty();
    }
    

    }

      
    (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.

  • AdT (unregistered) in reply to Web 1.0
    Anonymous:
    	if( (currentValue == null && newValue != null) ||
    (currentValue != null && newValue == null) )
    Why not simply
    if ((currentValue == null) != (newValue == null))
  • (cs) in reply to AdT
    Anonymous:
    DagSverre:

    If you want a "nice oneliner" syntax then dirty = dirty && ... will do it

    Nope. You have one more guess what the correct operator is.


    Ouch! That should teach me something about commenting the mistakes of others :-)
     

  • Yuriy (unregistered) in reply to Anonymous

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

    And look up short-circuiting before telling me it'll throw an NPE.

    TRWTF are the comments.

    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;


  • (cs) in reply to DagSverre
    DagSverre:

    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.

    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.

     

  • (cs) in reply to Anonymous

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

    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.

  • Kobes (unregistered)

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

  • (cs) in reply to Earl Purple
    Earl Purple:

    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.

     

    And yes, sometimes you might need to allow more than T/F/null, but you need to exercise reasonable judgment as to whether this need is likely to actually occur for a given case - otherwise you get code bloat, with a whole bunch of separate enums that are all basically equivalent to a boolean choice, and most of which will likely stay that way. 

     

    In the case of "setIsRecordLocked(Boolean newValue)", it's obvious what the boolean values stand for.  That is not the WTF.

    chrismcb:

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

    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.

     

    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()

    }

     

  • (cs)

    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.

  • Peter Lawrey (unregistered)
    // I agree, Boolean comparison is ugly...
    if (isRecordLocked != null ? !isRecordLocked.equals(newValue) : newValue != null) {
        context.setDirty(true);
        isRecordLocked = newValue;
    }
    
  • Anonymous (unregistered) in reply to Yuriy
    Anonymous:

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

    And look up short-circuiting before telling me it'll throw an NPE.

    TRWTF are the comments.

    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;




    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.
  • Anonymous (unregistered) in reply to Anonymous
    Anonymous:
    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!

    Blast! Just re-read the documentation. It also says that x.equals(null) must always return false. So you win.

    Captcha = truthiness, at last.
  • Stephanos Piperoglou (unregistered) in reply to Ottokar
    Ottokar:
    Hello,

    a tristate Boolean makes sense. For example if you have to deal with databases. DBs can store booleans with a NULL value.


    Absolutely true, but ironically, JDBC offers no way to read a Boolean from a ResultSet - only a primitive boolean. Bring out the 

    rs.isNull(column) ? null : new Boolean(rs.getBoolean(column));

    Regardless, your point brings up the question, why would you support tri-state booleans in the db in the first place? Tri-state booleans are semantically useful when reading from a database column, but more so when reading one marked NOT NULL: Boolean.TRUE means it's true, Boolean.FALSE means it's false, and null means the field hasn't been read from the database yet ;-)

  • Web 1.0 (unregistered) in reply to emurphy

    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.

  • Web 1.0 (unregistered) in reply to Web 1.0

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

  • IcemanK (unregistered) in reply to Stephanos Piperoglou
    Anonymous:
    Ottokar:
    Hello, a tristate Boolean makes sense. For example if you have to deal with databases. DBs can store booleans with a NULL value.

    Absolutely true, but ironically, JDBC offers no way to read a Boolean from a ResultSet - only a primitive boolean. Bring out the 

    rs.isNull(column) ? null : new Boolean(rs.getBoolean(column));

    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. 

  • Olddog (unregistered) in reply to IcemanK


    Just curious... after several pages of excellent boolean arguments, why would it matter if "isRecordLocked" was dirty or not?

  • Anonymous (unregistered) in reply to Why?
    Anonymous:

    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

    After reading 4 pages of comments, I have to disagree.  This is the best WTF I've seen in a long time.
     

  • Jonathan (unregistered)
  • Jerry (unregistered) in reply to AaronStJ

    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.
     

  • Ben M (unregistered) in reply to Divy

    don't u mean?:

    public void setIsRecordLocked(Boolean newValue) {
        if (isRecordLocked != newValue)
        {
            context.setDirty(true);

        } else {
            isRecordLocked = newValue;
        }
    }

  • Weasel™ (unregistered)

    LMFAO

  • anonymous koder (unregistered) in reply to JamesCurran

    You are right, it does not call setDirty if both newValue and isRecordLocked hold the same value (true ot false).

    My fault!!

     

    [image] 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.

     

  • correct (unregistered)

    In Java objects can be null so you should already have one of these:



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


    Then you can rewrite the original method like so:


    public void setIsRecordLocked(Boolean newValue) {
        if ( !Objects.equal( isRecordLocked, newValue ) {
            context.setDirty(true);
            isRecordLocked = newValue;
        }
    }


    You will probably find many times for reusing this utility method and improves readability a great deal.



     

  • Henry (unregistered) in reply to jaspax

    How about this?

    if(newValue!=NULL) { if (isRecordLock==NULL or !isRecordLock.Equals(newValue)) { context.SetDirty(True) isRecordLock=newValue; } }

  • Brandito (unregistered) in reply to donazea

    if (!(isRec == newVal == null || isRec == newVal != null)) setDirty(true);

    isRec = newVal;

  • Cessair (unregistered)

    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:

    • I'm sorry, guys
    • we were forced to use objects rather than primitive classes
    • I'm pretty sure it was actually C++, not Java
    • it was definitely pre-Google and pre-WTF, probably about 1996
    • it took AAAAGES to work out how to do it, and many of your arguments and questions went through my head at the time
    • I don't code any more....
  • markm (unregistered)

    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); }

Leave a comment on “setDirty( true )”

Log In or post as a guest

Replying to comment #106287:

« Return to Article