Clarity thru ternary operators ? true : false



  • I love the ternary operator, but not in this case, from a C# app:

    btnFoo.Enabled = ((chkSimulateBar.Checked || _isDbConnected)  && (txtBletchl.Text.Trim() != "")) ? true : false;
    


  • is this real code or are you just putting interview questions?

    @rstinejr said:


    I love the ternary operator, but not in this case, from a C# app:

    btnFoo.Enabled = ((chkSimulateBar.Checked || _isDbConnected)  && (txtBletchl.Text.Trim() != "")) ? true : false;
    


  •  The amusing part is the author probably thought he was being smart. Not like those other developers that would have used a standard if/else.



  • I've done this same thing before, when I wasn't thinking about it. I usually caught myself before committing, though.



  • @Nagesh said:

    is this real code or are you just putting interview questions?

    @rstinejr said:


    I love the ternary operator, but not in this case, from a C# app:

    btnFoo.Enabled = ((chkSimulateBar.Checked || _isDbConnected)  && (txtBletchl.Text.Trim() != "")) ? true : false;
    

    It's real. I copied it from code to which I'm adding features, although I changed the variable name roots to "Foo", "Bar", etc., because somehow that seemed like the thing to do.



  • I hope you refactored it to

    btnFoo.Enabled = true ? ((chkSimulateBar.Checked || _isDbConnected)  && (txtBletchl.Text.Trim() != "")) : false;
    

    in order to raise even more eyebrows in the future.



  • @rstinejr said:


    I love the ternary operator, but not in this case, from a C# app:

    btnFoo.Enabled = ((chkSimulateBar.Checked || _isDbConnected)  && (txtBletchl.Text.Trim() != "")) ? true : false;
    

    That doesn't seem so bad. I mean, it's only one level deep, right? I'm sure most of us havee seen (and, in all likeliness, written), worse...



  • @jpa said:

    I hope you refactored it to

    btnFoo.Enabled = true ? ((chkSimulateBar.Checked || _isDbConnected)  && (txtBletchl.Text.Trim() != "")) : false;
    

    in order to raise even more eyebrows in the future.

    +1


  • Considered Harmful

    btnFoo.Enabled = ((((chkSimulateBar.Checked ? true : false) || (_isDbConnected ? true : false) ) ? true : false) && ((txtBletchl.Text.Trim() == "" ? false : true ))) ? true : false;



  • @rstinejr said:

    btnFoo.Enabled = ((chkSimulateBar.Checked || _isDbConnected)  && (txtBletchl.Text.Trim() != "")) ? true : false;

    Arg, that is such a pet peeve of mine!

    btnFoo.Enabled = ((chkSimulateBar.Checked || _isDbConnected)  && (txtBletchl.Text.Trim().Length != 0)) ? true : false;
    

    Much better!



  • @jpa said:

    I hope you refactored it to

    btnFoo.Enabled = true ? ((chkSimulateBar.Checked || _isDbConnected)  && (txtBletchl.Text.Trim() != "")) : false;
    

    in order to raise even more eyebrows in the future.

    Reminds me of Python's "value_if_true if condition else value_if_false" ternary madness.



  • btnFoo.Enabled = ((chkSimulateBar.Checked or _isDbConnected) and (len(txtBletchl.Text.strip()) > 0)) if True else False
    

    You mean like this?



  • @Xyro said:

    @rstinejr said:
    btnFoo.Enabled = ((chkSimulateBar.Checked || _isDbConnected)  && (txtBletchl.Text.Trim() != "")) ? true : false;

    Arg, that is such a pet peeve of mine!

    btnFoo.Enabled = ((chkSimulateBar.Checked || _isDbConnected)  && (txtBletchl.Text.Trim().Length != 0)) ? true : false;
    

    Much better!

    If yer gonna correct it, at least do it right:

    btnFoo.Enabled = ((chkSimulateBar.Checked || _isDbConnected)  && (!string.IsNullOrWhitespace(txtBletchl.Text))) ? true : false;
    


  •  That line of code is too long and has too many non-ternary operators! Correct way:

     

    btnFoo.Enabled = false ? true : chkSimulateBar.Checked
    btnFoo.Enabled = _isDbConnected ? true : btnFoo.Enabled
    
    btnFoo.Enabled = string.IsNullOrWhitespace(txtBletchl.Text) ? false : btnFoo.Enabled;
     

     



  • @superjer said:

     That line of code is too long and has too many non-ternary operators! Correct way:

     

    btnFoo.Enabled = false ? true : chkSimulateBar.Checked
    btnFoo.Enabled = _isDbConnected ? true : btnFoo.Enabled
    btnFoo.Enabled = string.IsNullOrWhitespace(txtBletchl.Text) ? false : btnFoo.Enabled;
     

     

    You shouldn't be using the btnFoo.Enabled as your personal temporary space.

    btnFoo.Enabled = string.IsNullOrWhitespace(txtBletchl.Text) ? false : (_isDbConnected ? true : (false ? true : chkSimulateBar.Checked));


  •  I have genuinely seen lines like this:

    foo = bar ? false : true

    and

    foo = (bar == false) ? true : false



  • @jpa said:

    I hope you refactored it to

    btnFoo.Enabled = true ? ((chkSimulateBar.Checked || _isDbConnected)  && (txtBletchl.Text.Trim() != "")) : false;
    

    in order to raise even more eyebrows in the future.

     

    Brillant! Then in the next commit, you can change it to

    btnFoo.Enabled = false ? true: ((!chkSimulateBar.Checked && !_isDbConnected) || txtBletchl.Text.Trim() == ""); 

    or something even more confusing like

    btnFoo.Enabled = false ? (chkSimulateBar.Checked && _isDbConnected && txtBletchl.Text.Trim() != ""): ((!chkSimulateBar.Checked && !_isDbConnected) || txtBletchl.Text.Trim() == ""); 

    This pattern must be promoted. Eyebrows will be raised!



  • @rstinejr said:

    btnFoo.Enabled = ((chkSimulateBar.Checked || _isDbConnected)  && (txtBletchl.Text.Trim() != "")) ? true : false;
    

    is the WTF that it's not

    btnFoo.Enabled = ((chkSimulateBar.Checked || _isDbConnected) && (txtBletchl.Text.Trim() != ""))



  • @russ0519 said:

    @rstinejr said:

    btnFoo.Enabled = ((chkSimulateBar.Checked || _isDbConnected)  && (txtBletchl.Text.Trim() != "")) ? true : false;
    

    is the WTF that it's not

    btnFoo.Enabled = ((chkSimulateBar.Checked || _isDbConnected) && (txtBletchl.Text.Trim() != ""))

    That's the WTF.





  • @Jonathan said:

    BooleanUtils.java

    There could be a plausible reason for the existence of that class. Right?



  • Ouch. If true, return true. If false, return false. Yeah, that's worth obscuration!



  • @Jonathan said:

    BooleanUtils.java

    Wow this thing is so stupid its almost funny.
    Here are some the highlights I found particularly insane (Moreso than the others, at least..)
    466, 507:
    Are literally wrappers around the ternary operator.

    649:
    Wins points for being the most bass-ackwards parseBoolean method ever (Including a direct string comparison-by-reference that will never be used!)
    It's seriously hard to be shittier than that thing is.

    906:
    My god. This method is called 'xor'. A better name for it would be 'array_contains_exactly_one_true_value'. Or to be truly Java-like: 'arrayContainsExactlyOneTrueValue'.



  • @morbiuswilters said:

    @Jonathan said:
    BooleanUtils.java

    There could be a plausible reason for the existence of that class. Right?

     

    The documentation suggests it's for "Beans". That was intended to be "type agnostic" aka nearly brain-dead, so it might well need some huge class to handle with difficult cases such as checkboxes. Not bad per se, but nobody should use it until forced at gun point.



  • @Salamander said:

    466, 507:

    Are literally wrappers around the ternary operator.

    Except with integers and Integers returns instead of booleans, so not really. (In Java, there is no automatic conversion of anything to booleans (except Booleans -- boxed), this includes integers and nulls.) As a part of a commons library, this is useful.

    @Salamander said:

    649:

    Wins points for being the most bass-ackwards parseBoolean method ever (Including a direct string comparison-by-reference that will never be used!)

    It's seriously hard to be shittier than that thing is.

    Why the harshness? Want to see what the Sun Oracle implementation is? This is from JRE 7, but has been unchanged since whenever they first released the source:

        private static boolean toBoolean(String name) {
            return ((name != null) && name.equalsIgnoreCase("true"));
        }
    
    You might think that to be simple, obvious, straightforward, and thusly superior, right? But if you dig into the equalsIgnoreCase(), and subsequent regionMatches(), and subsequent call to Character.toUpperCase() for every single matching character, and subsequent CharacterData lookup and the following Unicode garbage to find the upper cased version, you'll find that the Apache Commons code is indeed far more efficient, even if it looks goofy. The method you mock is in a library for extremely common operations, the kind that could very well be found in an inner loop. If you ask me, the "bass-ackwards" parseBoolean method is not only efficient and justified, but probably optimal! What would you come up with?

    @Salamander said:

    906:

    My god. This method is called 'xor'. A better name for it would be 'array_contains_exactly_one_true_value'. Or to be truly Java-like: 'arrayContainsExactlyOneTrueValue'.


    No comment.



  • @Xyro said:

    Except with integers and Integers returns instead of booleans, so not really. (In Java, there is no automatic conversion of anything to booleans (except Booleans -- boxed), this includes integers and nulls.)

    But... the ternary operator doesn't return booleans, except for WTF examples as the OP. And it is literally a wrapper, you call
    toBoolean( bool , trueValue , falseValue);
    instead of writing
    bool ? trueValue : falseValue;
    And the name is misleading, too, as it returns an integer based on a boolean value, while I would expect it to convert an object or whatever to a boolean...
    So, still a WTF



  • @dargor17 said:

    But... the ternary operator doesn't return booleans, except for WTF examples as the OP. And it is literally a wrapper, you call
    toBoolean( bool , trueValue , falseValue);
    instead of writing
    bool ? trueValue : falseValue;
    And the name is misleading, too, as it returns an integer based on a boolean value, while I would expect it to convert an object or whatever to a boolean...
    So, still a WTF

    Eh, you're right. I misunderstood the intent of the code, the examples in the javadoc threw me off. I thought the point was to turn a boolean into a 1 or a 0, but I just read it too fast. It is indeed a ternary wrapper.



  • @Xyro said:

    The method you mock is in a library for extremely common operations, the kind that could very well be found in an inner loop. If you ask me, the "bass-ackwards" parseBoolean method is not only efficient and justified, but probably optimal! What would you come up with?

    First I'd profile that shit to see if it is even actually a problem. If it is, that's when you check to see if you can remove the source of the problem (Read: What the hell is parsing strings to booleans so often?).
    If you can't fix that, then take a look at the data that's being parsed: are they all a particular case? Can you get away with using a standard case-sensitive equals call once or twice?
    Basically what I'm saying is: You should only be rewriting the shit the standard libraries already do if you have run out of every other option, and since you are rewriting it for a specialised case you should not include it in a generalised library.



  • But it's not a specialized case, and it is a generalized library, and it does more than the standard library (by accepting "yes" and "on", as dubious as those values are). I'd imagine a very common use for such a parsing method is dealing with config files, such as in Apache Configuration. So if you're going to write it, and it's going to be used all over the place (directly or indirectly), what's so terrible with writing it like that?

    I would never claim that parsing strings of a couple of characters could ever show up on a chart of bottlenecks, but why not make it efficient anyway? We COULD have it hooked up to an enterprise-class database that serializes boolean strings to over-engineered XML through third-party cloud services, and maybe it still wouldn't really be a bottleneck, but really why not just do it in a neat efficient manner? It's not even that "clever".


  • Discourse touched me in a no-no place

    @Xyro said:

    The method you mock is in a library for extremely common operations, the kind
    that could very well be found in an inner loop. If you ask me, the
    "bass-ackwards" parseBoolean method is not only efficient and justified, but
    probably optimal! What would you come up with?
    Profiling, if it was indeed an issue. And, subsequently - if it was, wonder why something is repeatedly parsing strings in an inner loop that is purportedly (and probably should be) using 'native' booleans.



  • @Xyro said:

    But it's not a specialized case, and it is a generalized library, and it does more than the standard library (by accepting "yes" and "on", as dubious as those values are). I'd imagine a very common use for such a parsing method is dealing with config files, such as in Apache Configuration.

    If you're rewriting existing functionality for non-specialised cases, then you're reinventing the wheel and being retarded. If you're extending functionality, you should still be using the existing method somewhere in there.
    Parsing strings to booleans may be a common function in a config parser, but you have to be fucking up really hard for it to be getting called so often to not only dwarf the IO, but to also become noticeably slow.

    @Xyro said:

    So if you're going to write it, and it's going to be used all over the place (directly or indirectly), what's so terrible with writing it like that?

    It doesn't strike you as odd that its about 40 longer than it needs to be for the sake of dubious efficiency concerns?

    @Xyro said:

    but really why not just do it in a neat efficient manner?

    That method isn't neat, and neither are useless dependencies. They are irritating, and the person who next comes along will look at it and go "why the hell is this even here? Does this even work correctly?".

    As for how I'd write it to extend its functionality, return Boolean.parseBoolean(str) || "yes".equalsIgnoreCase(str) || "on".equalsIgnoreCase(str);
    Then I'd move on to something worth the time; rewriting the stupid parts of the program that suffer crippling slowdowns parsing worst-case four letter strings seems like a good place to start.


Log in to reply