• ... (unregistered)

    wow, good thing that isn't in the military!

  • (cs)

    I'm thinking and thinking and can't figure out the logic in that piece of code. Am I alone in this?

  • (cs) in reply to felix
    felix:

    I'm thinking and thinking and can't figure out the logic in that piece of code. Am I alone in this?

    This method sets isRecordLocked; if the new value is different from the old value, it also sets isDirty to true to mark the object as "dirty".

  • Some Guy (unregistered)

    I think this was machine generated code.

  • (cs)

    Perhaps I'm dumb, but I can't find a way to put this into a very succinct one-liner. If you don't need to worry about isRecordLocked being null, then you can just do:

     

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

    There's various ways to handle the incoming value that ensure that it never generates a NullPointerException, but none of them are necessarily more obvious than what's given here.

     If it were up to me, I'd just change Boolean to boolean and write "if (isRecordLocked != new Value) { ... }".  It would take about ten minutes with the "Refactor" function in Eclipse.

  • KNY (unregistered) in reply to Some Guy

    Anonymous:
    I think this was machine generated code.

    Machines everywhere are insulted by this post. 

  • whitty (unregistered)

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

    The code isn't that bad in the scheme of WTFs.  At least it explicitly covers the right bases.

    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.

  • Divy (unregistered) in reply to whitty

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

     

    what abt just

     context.setDirty(true);
     isRecordLocked = newValue;

     

    assuming the assumption that a null can't be equal to non-null is true :)
     

  • Erwan (unregistered) in reply to jaspax
    jaspax:

    Perhaps I'm dumb, but I can't find a way to put this into a very succinct one-liner. If you don't need to worry about isRecordLocked being null, then you can just do:

     

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

    There's various ways to handle the incoming value that ensure that it never generates a NullPointerException, but none of them are necessarily more obvious than what's given here.

     If it were up to me, I'd just change Boolean to boolean and write "if (isRecordLocked != new Value) { ... }".  It would take about ten minutes with the "Refactor" function in Eclipse.

     

    You would then loose a big feature of this code : it uses 3-valued booleans (true, false and null).


  • Mr Unanimous (unregistered) in reply to KNY

    This should do the same thing, right? The potential for changing isRecordLocked to null seems bad, but who knows, maybe it's supposed to for some reason. :)

    What's the difference between boolean and Boolean?

     

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

        // Change the value.
        isRecordLocked = newValue;
    }
     

  • (cs)

    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.

  • lplatypus (unregistered) in reply to whitty

    Anonymous wrote:

    if (isRecordLocked != newValue)

    No! In Java, Boolean is a class, not a simple type like bool, so:

    1. 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.
    2. 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))

     

  • Steve (unregistered) in reply to lplatypus

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

    Is this right?  I thought boolean short-circuiting will mean this is totally different from the original.   For clarity (not brevity) I would be tempted to transfer the nulls to false's via a temporary to make the logic more obvious.   Yes it's a waste.

     

     

  • Jon Haugsand (unregistered) in reply to lplatypus
    Anonymous:

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

     

     Really?  I cannot get this to work.  E.g. if both are null, should the dirty flag really be set?
     What we need for clearity, is a small abstraction, something that should exist in the Object class:

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

     

    Then we just need to say:

       if (! Object.equals(isRecordLocked, newValue) ) context.setDirty(true);

     

     

  • Jamie (unregistered)

    NoooooooOOOOooooooooooooOoooooooooooooooooOOOOOOOOooooOOOOOOOOoooOOOOOOOO!

     

    I thought maybe, just maybe, New Zealand might be some kind of safe haven from WTFs (this is the first one I have seen on here). *sigh* I could see it being IRD, or maybe studylink. All our government has done with new technology is move the queues from physical locations to phones and the internet. It still takes weeks to get ANYTHING done. Knowledge wave my ass.

     

    I need to start preparing myself for crud like this I suppose.  

  • Steve (unregistered) in reply to Steve

    Actually I just figured, that that's wrong too because in the case of isLocked==false and newValue==null, which would result in setDirty(true), my new code wouldn't work.

    Unless I'm missing something fundamental there is no easy way to do this to get the same answer as the original unless Boolean.valueOf() was used to create the booleans.   In which case the newValue == isLocked can be used because all the TRUE/FALSE values will be the same object.   I'm actually starting to prefer the WTF solution ...
     

  • AaronStJ (unregistered)

    public void setIsRecordLocked(Boolean newValue) {

        context.setDirty(isRecordLocked != newValue);

        isRecordLocked = newValue; 

    } 

  • (cs)

    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 is a great opportunity to use the trinary conditional operator :) Here's the simplest equivalent code I can come up with:

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

    It appears that .equals(null) always returns false on a Boolean, so I removed the redundant newValue == null check.

  • mcosta (unregistered)

    All you are missing a thing. If both are not null and different two calls to setDirty() are made. That's why he says " Do not try to use this code as a pick up line".

  • TK (unregistered)

    I think the original WTF is fairly good code. All the the versions suggested in above comments a wrong in some way or other. Using Boolean instead of Java's built in type boolean creates this kind of hoop to jump through.

    There are advantages of using Boolean, for example you can store them in data structures that take generic "Object". Or they use it just for the enterpriseyness and to please some OO fanatic. I wonder if they use Integer instead of int too...

  • Jon Haugsand (unregistered) in reply to AaronStJ
    Anonymous:

    public void setIsRecordLocked(Boolean newValue) {

        context.setDirty(isRecordLocked != newValue);

        isRecordLocked = newValue; 

    } 

     Which won't work for two reasons.  First is that you should not reset the dirty flag if it is already set to true.  The second is that isRecordLocked is always different from newValue if one of them is generated by using "new Boolean(whatever)".

     
    The real problem with the code is actually not the incomprehensive code, but the three value logic of it.  Assuming here that there is no real semantic of a Boolean null, the programmer should assume that null is not possible, and use aspect oriented programming or some plain assert statements in the beginning of the function:

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

    Even better is perhaps to assume that both isRecordNull and newValue is made through Boolean.valueOf or Boolean.{TRUE,FALSE}:

     

    assert(newValue==Boolean.TRUE || newValue==Boolean.FALSE);

    assert(isRecordLocked==Boolean.TRUE||isRecordLocked==Boolean.FALSE);

    if (newValue != isRecordLocked) context.setDirty(true);

     

    This, however, would probably break "working" code.



     

  • TK (unregistered) in reply to mcosta

    Anonymous:
    All you are missing a thing. If both are not null and different two calls to setDirty() are made. That's why he says " Do not try to use this code as a pick up line".

    No, the second one is else if. 

  • Smurf (unregistered) in reply to AaronStJ
    Anonymous:

    public void setIsRecordLocked(Boolean newValue) {

        context.setDirty(isRecordLocked != newValue);

        isRecordLocked = newValue; 

    } 

    Umm, no. The assumption is that these are objects and that you need to call .equals to find out if the content is the same, instead of != which just checks if it's the same object. You might also be unable to pass a NULL object to .equals().

    If that is so, then the original code is in fact correct. An equivalent one-liner test might be

    if (old ? new ? !old.equals(new) : TRUE : new != NULL)

    which is somewhat more readable. 

  • (cs) in reply to whitty

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

    The code isn't that bad in the scheme of WTFs.  At least it explicitly covers the right bases.

    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.

    Maybe they wanted to add a FileNotFound state

  • berrs (unregistered)

    Maybe it's because I just woke up, but I cannot get the WTF in this. Ok, it takes a while to grasp but I see nothing wrong. If the new value is different from the old value, setDirty(true). So? But I would like to see that one-liner.

     

  • Remco Gerlich (unregistered) in reply to berrs

    Let's give it a try too...

     

    public void setIsRecordLocked(Boolean newValue) {

        if (newValue == null ? (isRecordLocked != null) : !newValue.equals(isRecordLocked)) {

          setDirty(true);

       }
     

        isRecordLocked = newValue; 

    }

     

    But there's not much wrong with the original. I much prefer the static Object.equals that someone proposed, given that that doesn't exist, you need to have it in a library in your project somewhere. It's the only readable way.
     

  • (cs) in reply to Divy

    Hmmm. This is not really a very spectacular WTF. I wouldn't say "WTF?!" at all if I'd find something like this.

    Anonymous:

     what abt just

     context.setDirty(true);
     isRecordLocked = newValue;

    That's wrong because setDirty() is not called in the original code when both isRecordLocked and isDirty are null. So your code does something different than the original code.

  • dnnrly (unregistered)

    The WTF is that it looks wrong, but when you get down to it I reckon it's right. It's just one of those times where you have to step away from the code and let someone smarter than you fiddle with it.

    As far as I can see, it checks that neither of the Boolean objects are null before comparing the values and also comparing the 'nullness' too. I wouldn't like to fiddle with this until I knew what context.setDirty(...) did.

  • (cs)

    Hello,

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

    Regards

  • (cs)

    Can it be true? The original code is better than most of the comments!

    This is the funniest in days - everyone trying to be smart and do it better, and almost every one trying to post a better solution getting it wrong. The original code is actually the best attempt I've seen yet (but I did skim it so I probably missed a clear-thinking soul here and there). Well, ok, it is probably a WTF to accept null as a value in this circumstance, but given that behaviour (treat null as a value, and changing to/from null should set the dirty flag) the original code beats most of the following attempts.

    The real WTF is variants of

     context.setDirty(isRecordLocked.equals(newValue))

    (ignoring null issues)

  • (cs) in reply to Goplat
    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.

  • sprx (unregistered) in reply to Goplat
    Goplat:

    This is a great opportunity to use the trinary conditional operator :) Here's the simplest equivalent code I can come up with:

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

     

    Best solution I've seen so far. Most other code pieces will result in a NullPointerException.

    Because

    if(isLocked != null || isLocked.doSomething()) is equal to if(null != null || null.doSomething()) 

     if isLocked is null of course.

     

    CAPTCHA = paula 

  • Slacker (unregistered) in reply to Ottokar

    That's so stupid it's almost funny

  • (cs) in reply to Ottokar

    The original code is not so terrible except that it would be useful to have a function to compare tri-state booleans. So how about I write one

    static public boolean equalsTristateBoolean( Boolean first, Boolean second )
    {
       return (( first == null && second == null )) || ( first != null && second != null && first.equals( second ) );
    }

    Put the above in ourUtil (our utilities) Now the original code could be something like:

    public void setRecordLocked( Boolean newValue )
    { 
        if ( ! ourUtil.equalsTristateBoolean( isRecordLocked, newValue ) )
        {
            context.setDirty( true );
            isRecordLocked = newValue;
        }
    }

     

  • Bart (unregistered) in reply to Wiggles
    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.

     Unfortunately, you can't autounbox a null Boolean.  The result will be a NullPointerException.

     
    The original code is messy because the "flags" are three state variables that probably should have been primitive types rather than Objects.  Most likely the developer is using null as an uninitialized indicator rather than maintaining two separate flags for the value and the "is initialized" state.

    You could rewrite it as:

    if( newValue == null && isRecordLocked == null )
    {
      return;
    }

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


    This is logically equivalent to the original but maybe a little more readable.  For completeness, a horrible but functional way to write it:


    boolean is_unchanged = false;

    try
    {
      is_unchanged = (newValue == isRecordLocked || newValue.equals(isRecordLocked))
    }
    catch (NullPointerException e) {}

    if( !is_unchanged )
    {
      setDirty(true);
      isRecordLocked = newValue;
    }

  • nts (unregistered)

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

        //the same - do nothing 

    } else {

        context.setDirty(true);

        isRecordLocked = newValue;

    } 

  • (cs) in reply to Slacker

    Well,

    what is that funny?

  • (cs) in reply to Bart
    Anonymous:


    if( newValue == null && isRecordLocked == null )
    {
      return;
    }

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

     

    Well,

     

    that looks as the best solution I have seen so far.

     What about this:

     

     

    if( newValue ==  isRecordLocked ) {
      return;
    }

    if( newValue == null || isRecordLocked == null )
    {
      setDirty(true);
    }

    isRecordLocked = newValue;

  • Anonymous (unregistered)

    Alex could keep this site running indefinitely by generating the day's WTF from the previous day's comments.

  • Asd (unregistered)

    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;
    }
  • Asd (unregistered) in reply to Asd
    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;
    }

    D'oh. I am one of the idiots. Of course the setDirtyIfDifferent method should be:

    if(!ObjectUtils.equals(obj1,obj2)) {

     setDirty(true);

    } 

  • (cs) in reply to Anonymous

    Anonymous:
    Alex could keep this site running indefinitely by generating the day's WTF from the previous day's comments.

     I Agree. :)
     

  • (cs) in reply to Wiggles
    Here's a way to test it...

    public class test {
        private static Boolean isRecordLocked;
        private static Boolean choices1[] = {null, new Boolean(true), new Boolean(false)};
        private static Boolean choices2[] = {null, new Boolean(true), new Boolean(false)};
       
        public static void main(String[] args) {
            for (int func = 0; func < 4; func++) {
                for (int i = 0; i < 3; i++) {
                    for (int j = 0; j < 3; j++) {
                        System.out.print("old=" + choices1[i] + ", new=" + choices2[j] + ", ");
                        isRecordLocked = choices1[i];
                        switch (func) {
                            case 0: setIsRecordLocked(choices2[j]); break;
                            case 1: setIsRecordLocked1(choices2[j]); break;
                            case 2: setIsRecordLocked2(choices2[j]); break;
                        }
                        System.out.println();
                    }
                }   
                System.out.println();
            }
        }
       
        public static void setIsRecordLocked(Boolean newValue) {
            // Update state if required.
            if (isRecordLocked != null) {
                if (newValue == null || !isRecordLocked.equals(newValue)) {
                    System.out.print("setDirty!");
                }
            }
            else if (newValue != null) {
                System.out.print("setDirty!");
            }

            // Change the value.
            isRecordLocked = newValue;
        }

        public static void setIsRecordLocked1(Boolean newValue) {
            if (isRecordLocked != null) {
                if (!isRecordLocked.equals(newValue)) {
                    System.out.print("setDirty!");
                }
            }
            else if (newValue != null) {
                System.out.print("setDirty!");
            }
            isRecordLocked = newValue;
        }

        public static void setIsRecordLocked2(Boolean newValue) {
            if (isRecordLocked != newValue) {
                System.out.print("setDirty!");
            }
            isRecordLocked = newValue;
        }
    }


    The 1st is the original. The 3rd one shows you can't do a simple "!=".

    The 2nd one is a slightly simplified version of the original. The only difference is the "newValue == null" check is redundant, since if newValue IS null it will NOT be equal to isRecordLocked, because we already checked that isRecordLocked is definitely NOT null.

    This is the best I can do... if anyone has a simpler function that passes the test (using Boolean types, not boolean primitives), let me know. If not, as far as I'm concerned this code is perfectly fine except that one little redundant "newValue == null" check.

    (This is all assuming the original code was in Java and wasn't translated from another language...)
  • (cs)

    There are a couple of utility classes that would certainly help in this case. Check out HashCodeUtil and EqualsUtil on Google and you get the idea.

    The main reason for these "one-liners" to go through specific utility classes is that you don't want any nullpointers to occur in equals or hashCode functions, that is just a very costly mistake to make (either in production or during development).

    The other reason is that you can change system policy for Dates for example using these utility functions. For example a function "isGreatherThan" for two dates.

    What if the source date is null, does that mean infinity or actually null?  How should the second date compare to the first?  A utility function makes this policy consistent, rather than an invocation of the same functionality in each place. I'm recently looking at these simple "one-liners" and find that a utility helper function is a great help for establishing these policies. Also, as soon as you want to construct objects and wish to add some functionality, changing the function prototype with modern IDE's will automatically flag any instances where these objects are being used.

    For the above, I'd actually consider using proxies, interceptors or AOP and see how this performs against manually invoked code. For example, every object of interest could become wrapped automatically and contain this "dirty flag". Any method invoked that would change a property can then individually set the object to dirty without any concern to the programmer.

    Then again... the name "isRecordLocked" in code sort of bothers me. is this Enterprise thing trying to mimic a database or what?
     

  • (cs) in reply to Chiraz
    Chiraz:

    Then again... the name "isRecordLocked" in code sort of bothers me. is this Enterprise thing trying to mimic a database or what?

    Even if you use a database you have lock records sometimes. For example if someone wants to change a value over a webpage.

     
    Regards
     

  • another damn kiwi (unregistered) in reply to Slacker

    Im not really sure I understand the animosity to the Boolean object.

    The Boolean object can only have 2 values either true or false. When the object reference is null, it means we dont have a Boolean object yet (perhapes because at the moment we dont know what value it should take, so havent created it yet)

    Makes perfect sense to me, I cant see a third value in the Boolean object no matter how hard i look.

    On topic, I guess all the attempts show that sometimes what we think is a piece of cake, is sometimes really a can of worms. (Watties, extra cheesy, are my favorite.)

  • another damn kiwi (unregistered) in reply to Bart

    Anonymous:

    ...but maybe a little more readable

    ...

    if( !is_unchanged ) ...

    ZOMG WTF!!!

  • (cs) in reply to another damn kiwi
    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.

    Or in a general speaking: Whenever you have to create an empty object which will be filled with data by a different system (other computer, other object, other ...) you should set the data to a default NULL value. Otherwise you wont know whether the "fill up" worked or worked not. Thats what we call "defensive programing".

    Regards.
     

  • Asd (unregistered)

    Just to point out isRecordLocked may not mean Database style record locking. It could mean, for example, that your user account is locked, or maybe an article is locked for publishing.

  • (cs) in reply to Einsidler
    Einsidler:

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

    The code isn't that bad in the scheme of WTFs.  At least it explicitly covers the right bases.

    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.

    Maybe they wanted to add a FileNotFound state

    Yikes, I've been hanging round here too long, this was the first thing that popped into my head. (-: 

Leave a comment on “setDirty( true )”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article