Correct evaluation of expression

Question

I came across the following expression in someone else's code. I think it's terrible code for a number of reasons (not least because it fails to take into account bool.TrueString and bool.FalseString), but am curious as to how the compiler will evaluate it.

private bool GetBoolValue(string value)
{
    return value != null ? value.ToUpper() == "ON" ? true : false : false;
}

Edit Incidentally, aren't the expressions evaluated from the inside-outwards? In this case, what's the point of checking for value != null after the call to value.ToUpper() which will throw a null reference exception?

I think the following is a correct (deliberately) verbose version (I'd never leave it like this :D ):

if (value != null)
{
    if (value.ToUpper() == "ON") 
    {
        return true;
    }
    else        // this else is actually pointless
    {
        return false;
    }
}
else
{
    return false;
}

Which can be shortened to:

return value != null && value.ToUpper == "ON";

Is this a correct re-writing of the expression?

Answer 1

You are correct, both in your rewriting and in your assertion that this attempt at conciseness is bad because it leads to confusion.

Answer 2

I read the original expression the same way you do. So I think your shortened expression is correct. If value is null it will never get to the second conditional, so it looks safe to me.

Answer 3

well the first one is a double-nested ternary operator

return (value != null) ? [[[value.ToUpper() == "ON" ? true : false]]] : false;

The bit in [[[ ]]] is the first result of the ternary expression which gets evaluated when the first condition is true so you're reading/assertion of it is correct

but its ugly as hell and very unreadable/unmaintainable in its current state. I'd definitely change it to your last suggestion

SideNote:

People who do

if(X == true) 
   return true;
else
   return false;

instead of

 return X;

should be taken out and shot ;-)

Answer 4

I also hate the constructs like:

if (value.ToUpper() == "ON") 
{
    return true;
}
else        // this else is actually pointless
{
    return false;
}

as you noticed it is a long and convoluted (not to say stupid) way of writing:

return value.ToUpper() == "ON";

Your proposition is nice, short and correct.

Answer 5

Are you looking for speed or readability and organization? Speed of execution, your shortened example is probably the best way to go.

For a few extra milliseconds, you could re-write this utility method as an extension method like so:

public static bool ToBoolean(this string value)
{

    // Exit now if no value is set
    if (string.IsNullOrEmpty(value)) return false;

    switch (value.ToUpperInvariant())
    {
        case "ON":
        case "TRUE":
            return true;
    }

    return false;

}

... and then you would use it as follows:

public static void TestMethod()
{
    bool s = "Test".ToBoolean();
}

EDIT: Actually, I'm wrong... a quick performance test shows that the extension method is FASTER than the inline method. The source of my test is below, as well as the output on my PC.

[Test]
public void Perf()
{

    var testValues = new string[] {"true", "On", "test", "FaLsE", "Bogus", ""};
    var rdm = new Random();
    int RunCount = 100000;
    bool b;
    string s;

    Stopwatch sw = Stopwatch.StartNew();
    for (var i=0; i<RunCount; i++)
    {
        s = testValues[rdm.Next(0, testValues.Length - 1)];
        b = s.ToBoolean();
    }
    Console.Out.WriteLine("Method 1: {0}ms", sw.ElapsedMilliseconds);

    sw = Stopwatch.StartNew();
    for (var i = 0; i < RunCount; i++)
    {
        s = testValues[rdm.Next(0, testValues.Length - 1)];
        b = s != null ? s.ToUpperInvariant() == "ON" ? true : s.ToUpperInvariant() == "TRUE" ? true : false : false;
    }
    Console.Out.WriteLine("Method 2: {0}ms", sw.ElapsedMilliseconds);


}

Output:

Method 1: 21ms
Method 2: 30ms

Answer 6

It looks like the method is indended to handle a value that comes from a checkbox HTML element. If no value is specified for the checkbox, it uses the value "on" by default. If the checkbox is not checked there is no value at all from it in the form data, so reading the key from Request.Form gives a null reference.

In this context the method is correct, althought it's quite horrible due to the use of the if-condition-then-true-else-false anti-pattern. Also it should have been given a name that is more fitting for it's specific use, like GetCheckboxValue.

Your rewrite of the method is correct and sound. As the value is not culture dependant, converting the value to uppercase should not use the current culture. So a rewrite that is even slightly better than the one that you proposed would be:

return value != null && value.ToUpperInvariant == "ON";

(The culture independent methods are also a bit faster than the ones using a specific culture, so there is no reason not to use them.)

Incidentally, aren't the expressions evaluated from the inside-outwards?

If it was method calls so that all expressions were actually evaluated, they would, as the inner call has to be made to evaluate the parameters for the outer call.

However, the second and third operands of the conditional expression is only evaluated if they are used, so the expressions are evaluated from the outside and inwards. The outermost condition is evaluated first to decide which of the operands it will evaluate.

Answer 7

Another alternative:

return string.Equals( value, "ON", StringComparison.OrdinalIgnoreCase );