Misused Ternary



  • Cleaning up some C# code I found on Code Project that had a lot of problems but when I came across this one I just shook my head.

     

     pnlUserInfo.Enabled = (rdoSqlAuthentication.Checked) ? true : false;


  • Considered Harmful

    I especially like the single expression in parentheses.

    I think they wanted something more like   pnlUserInfo.Enabled = !((!(!rdoSqlAuthentication.Checked != true) : false : true) != false) == false;



  • Awesome. That's right up there with:

    if (a == true)
    {
      b = true;
    }
    else
    {
      b = false;
    }
    



  • Even better:

    pnlUserInfo.Enabled = (isTrue(rdoSqlAuthentication.Checked)) ? true : false;
    or
    pnlUserInfo.Enabled = (isTrue(rdoSqlAuthentication.Checked) ? true : false);

    By the way I thing there is nothing bad about parentheses(better readabiliti). I even sometimes use if(blah==true) ...




  • Your code-fu is weak.

    bool temp1 = ((isTrue(rdoSqlAuthentication.Checked)) ? !(rdoSqlAuthentication.Checked) : ((rdoSqlAuthentication.Checked) == false));
    bool temp2 = ((rdoSqlAuthentication.Checked && !temp1) ? ((temp1 = (rdoSqlAuthentication.Checked)) ? (isTrue(temp1) != true)  : (rdoSqlAuthentication.Checked ||  (temp1 ? false : (temp1 & rdoSqlAuthentication.Checked) == false))) : false);
    pnlUserInfo.Enabled = (!(temp2 == temp1) == false);



  • No no no! You guys have it all wrong. I'm [i]not making this stuff up[/i]. The following code is seriously part of our framework:

    if ((this.SomeBoolean == true) || (this.SomeObj.SomeBoolean == true))
    	return true;
    else
    	return false;
    



  • @djork said:

    No no no! You guys have it all wrong. I'm [i]not making this stuff up[/i]. The following code is seriously part of our framework:

     

    if ((this.SomeBoolean == true) || (this.SomeObj.SomeBoolean == true))
    return true;
    else
    return false;

    Nice. Does your system have a seperate function for every expression? I'm sure that makes things very clear...



  • @djork said:

    if ((this.SomeBoolean == true) || (this.SomeObj.SomeBoolean == true))
    return true;
    else
    return false;

    Alert! Alert!  Possible NullReferenceException.  Please re-write to:

    if ((this.SomeBoolean == true) || (this.SomeObj != null && this.SomeObj.SomeBoolean == true))
         return true;
    else
         return false;



  • @RaspenJho said:

    @djork said:

    if ((this.SomeBoolean == true) || (this.SomeObj.SomeBoolean == true))
    return true;
    else
    return false;

    Alert! Alert!  Possible NullReferenceException.  Please re-write to:

    if ((this.SomeBoolean == true) || (this.SomeObj != null && this.SomeObj.SomeBoolean == true))
         return true;
    else
         return false;

    Unless your object requires this.SomeObj to be non-null for this condition, or someObj is never null to begin with (f.ex final Class x /* assigned in constructor which dies if we get null */;), in which case it's entirely appropriate to let an exception be thrown. And even if not, your code assumes a null this.SomeObj should default to false, but without knowing class details, it could very easily be more appropriate to default to true.



  • if (this.SomeBoolean == true || this.SomeObj.SomeBoolean == true)

    return true;

    else if (this.SomeBoolean == false || this.SomeObj.SomeBoolean == false)

    return false;

    else

    return someobjnotfound;
     



  • Not defending the offending code.... but he may have read this:

     

    And been thinking along the lines of the ThreeState checkbox....

     

     



  • @aquanight said:

    @RaspenJho said:

    @djork said:

    if ((this.SomeBoolean == true) || (this.SomeObj.SomeBoolean == true))
    return true;
    else
    return false;

    Alert! Alert!  Possible NullReferenceException.  Please re-write to:

    if ((this.SomeBoolean == true) || (this.SomeObj != null && this.SomeObj.SomeBoolean == true))
         return true;
    else
         return false;

    Unless your object requires this.SomeObj to be non-null for this condition, or someObj is never null to begin with (f.ex final Class x /* assigned in constructor which dies if we get null */;), in which case it's entirely appropriate to let an exception be thrown. And even if not, your code assumes a null this.SomeObj should default to false, but without knowing class details, it could very easily be more appropriate to default to true.

    Yes, you are absolutely correct.  If you add some external conditions not mentioned by the OP, then you may not need to check for null.



  • I've often seen:

     

    if (someBool == true)

    someBool = false;

    else

    someBool = true; 

     

     

    Things like these I always change a leave a smiley face for the developer.

     

     



  • @flukus said:

    I've often seen:

     

    if (someBool == true)

    someBool = false;

    else

    someBool = true; 

     

    Things like these I always change a leave a smiley face for the developer.

    I think this is alright in its context.  The "== true" part may be logically extraneous, but it makes it very easy to see that someBool is being toggled.

     



  • so does  someBool = ! someBool ; in MUCH less effort!



  • Oh, all your lucky guys, you have wonderfull things like true and false.

    I am working with an obscure 4GL named Uniface that has a boolean data type, yes, but no "True" or "False"-Definition.
    Plus, it has automatic type conversion.

    All of these are valid "True"-Assignments:
    mybool = "T"
    mybool = "TRUE"
    mybool = "Y"
    mybool = 1
    mybool = -222
    mybool = "-222"

    All of these are valid "False"-Assignments:
    mybool = "F"
    mybool = "FALSE"
    mybool = "FileNotFound"
    mybool = "Fu**"

    mybool = "N"

    mybool = 0

    Conversion  gives some interesting effects:
    myNum = 123
    myBool = myNum
    myNum = myBool   ; is Now 1

    myStr =  "funthomas is great"

    myBool = myNum

    myStr = myStr        ; is Now "F"

    Also nice:
    myBool = "Y"
    if (myBool = "T")    ; resolve to True.

    But my favorite are typeless global register $1 - $99:
    $1 = "T"
    if ($1 = "Y")           ; resolve to False

    myBool = "T"
    $1 = myBool
    if ($1 = "Y")           ; resolve to True



  • @funthomas said:

    Oh, all your lucky guys, you have wonderfull things like true and false.

    I am working with an obscure 4GL named Uniface that has a boolean data type, yes, but no "True" or "False"-Definition.
    Plus, it has automatic type conversion.

    All of these are valid "True"-Assignments:
    mybool = "T"
    mybool = "TRUE"
    mybool = "Y"
    mybool = 1
    mybool = -222
    mybool = "-222"

    All of these are valid "False"-Assignments:

    mybool = "F"

    mybool = "FALSE"
    mybool = "FileNotFound"

    mybool = "Fu**"

    mybool = "N"

    mybool = 0

    Conversion  gives some interesting effects:
    myNum = 123
    myBool = myNum
    myNum = myBool   ; is Now 1

    myStr =  "funthomas is great"

    myBool = myNum

    myStr = myStr        ; is Now "F"

    Also nice:
    myBool = "Y"
    if (myBool = "T")    ; resolve to True.

    But my favorite are typeless global register $1 - $99:
    $1 = "T"
    if ($1 = "Y")           ; resolve to False

    myBool = "T"
    $1 = myBool
    if ($1 = "Y")           ; resolve to True

    Shiiiit... ya feel me?

    Sounds like you've found the next MUMPS 



  • Return else return!  Return else return!

    WHAT IS RETURN  ELSE RETURN?????

    Apparently the people that write this sort of thing don't understand what "return" does.

    If you're working with people that use "return else return" then either you need to leave, or they do.

    (The truly sad part is that the same construct is in K&R.  This is why Dennis Ritchie is not god.)



  • @mrprogguy said:

    Return else return!  Return else return!

    WHAT IS RETURN  ELSE RETURN?????

    Apparently the people that write this sort of thing don't understand what "return" does.

    If you're working with people that use "return else return" then either you need to leave, or they do.

    (The truly sad part is that the same construct is in K&R.  This is why Dennis Ritchie is not god.)

     

    Would you suggest creating a local variable just to set to true or false and then returning that after the if..else rather than simply returning as soon as you know what to return? *hides his stack* I'll be thanking you not to be creating variables all over for no reason.



  • @Kemp said:

    @mrprogguy said:

    Return else return!  Return else return!

    WHAT IS RETURN  ELSE RETURN?????

    Apparently the people that write this sort of thing don't understand what "return" does.

    If you're working with people that use "return else return" then either you need to leave, or they do.

    (The truly sad part is that the same construct is in K&R.  This is why Dennis Ritchie is not god.)

     

    Would you suggest creating a local variable just to set to true or false and then returning that after the if..else rather than simply returning as soon as you know what to return? *hides his stack* I'll be thanking you not to be creating variables all over for no reason.

    I believe he was talking about

    [code]return bool;[/code]

    or

    [code]if (cond) return ifTrue; return ifFalse[/code],

    sans the [code]else[/code].

    Seems like a style question for me. 

    That, or literally [code]if (china.rice_bag == fallen_down) return x else return x[/code] which would be a true WTF indeed.



  • Actually, it's been marked as a reply to

    if ((this.SomeBoolean == true) || (this.SomeObj.SomeBoolean == true))
    return true;
    else
    return false;

    which (ignoring the wtf condition and its interaction with the returns) is fine.

    Basically you can

    if (condition)
    return a;
    else
    return b;

    or

    if (condition)
    value = a;
    else
    value = b;
    return value

    Personally I prefer the first due it not creating unnecessary variables.



  • @Kemp said:

    Actually, it's been marked as a reply to

    if ((this.SomeBoolean == true) || (this.SomeObj.SomeBoolean == true))
    return true;
    else
    return false;

    which (ignoring the wtf condition and its interaction with the returns) is fine.

    Basically you can

    if (condition)
    return a;
    else
    return b;

    or

    if (condition)
    value = a;
    else
    value = b;
    return value

    Personally I prefer the first due it not creating unnecessary variables.

     

    I'm willing to bet a statistically insignificant amount of money that the point of the post was that it should be written this way:

     

    if(condition)
        return a;

    return b;

     

    It's the else causing him grief. Because of the way return works there's no need for an else.
     



  • @HitScan said:

    @Kemp said:
    Actually, it's been marked as a reply to

    if ((this.SomeBoolean == true) || (this.SomeObj.SomeBoolean == true))
    return true;
    else
    return false;

    which (ignoring the wtf condition and its interaction with the returns) is fine.

    Basically you can

    if (condition)
    return a;
    else
    return b;

    or

    if (condition)
    value = a;
    else
    value = b;
    return value

    Personally I prefer the first due it not creating unnecessary variables.

     

    I'm willing to bet a statistically insignificant amount of money that the point of the post was that it should be written this way:

     

    if(condition)
        return a;

    return b;

     

    It's the else causing him grief. Because of the way return works there's no need for an else.
     

    That's how I understood the post as well. I think return else return does have its use though in some cases. If the function is complicated and/or not really clearly structured, it can be confusing if the control flow is exited at unexpected points. ("Avoid multiple return" rule). In this case, it can increase readabillity if you mark your returns with if/else statements.

    I agree however, including unnecessary variables is worse than both methods. 



  • Ah, I see your point. I sometimes use the else myself, with the rule being that if the if statement has a large "true" clause then I put the second return separately, whereas a simple one like in the example I use an else with the return in that, just to emphasise that it's the alternate route.



  • I mostly just use [code]return[/code] alone if it's for a series of input checks at the start of a function. as in

    [code]
    if (!someCheck(input)) return;
    if (!someOtherCheck(input))) return;
    if (!yetAnotherCheck(input))) return;
    /* ... actual algorithm goes here ... */
    [/code] 



  • @death said:

    so does  someBool = ! someBool ; in MUCH less effort!
    I have seen, in a game I was 'improving', written in Lua, the following:

    if a == true then

        a = false

        return

    end

    if a == false then

        a = true

        return

    end

    this appeared more than once too. 



  • @djork said:

    No no no! You guys have it all wrong. I'm [i]not making this stuff up[/i]. The following code is seriously part of our framework:

     

    if ((this.SomeBoolean == true) || (this.SomeObj.SomeBoolean == true))
    return true;
    else
    return false;

     

    perhaps it's me, and i'm missing something but this could be perfectly valid.





    public function isActive() {
        if (this->hasPaid || this->account->isTrial) {
            return true
        } else{
            return false
        }
    }






  • @stratos said:

    perhaps it's me, and i'm missing something but this could be perfectly valid.

    public function isActive() {
        if (this->hasPaid || this->account->isTrial) {
            return true
        } else{
            return false
        }
    }

    public function isActive() {
        return this->hasPaid || this->account->isTrial;
    }
     



  • @PSWorx said:

    @stratos said:

    perhaps it's me, and i'm missing something but this could be perfectly valid.

    public function isActive() {
        if (this->hasPaid || this->account->isTrial) {
            return true
        } else{
            return false
        }
    }

    public function isActive() {
        return this->hasPaid || this->account->isTrial;
    }
     

     

    Yes it could be writen shorter, that doesn't make it wrong or stupid though. 



  • @stratos said:

    @PSWorx said:
    @stratos said:

    perhaps it's me, and i'm missing something but this could be perfectly valid.

    public function isActive() {
        if (this->hasPaid || this->account->isTrial) {
            return true
        } else{
            return false
        }
    }

    public function isActive() {
        return this->hasPaid || this->account->isTrial;
    }
     

     

    Yes it could be writen shorter, that doesn't make it wrong or stupid though. 

    Wrong in no case.  But I don't see how the former is in any way better. On the contrary, I find the latter much more concise and easier to understand. in my opinion, "return true else return false" is redundant without giving any additional benefit. Then again, I suppose there are other people who are not comfortable with boolean expressions outside of if-conditions. Maybe in cases like this it would make sense.


Log in to reply