Again on nested IF

Question

Sometimes I use something like:

private void Test()
{
    success = false;
    while (true)
    {
        if ( ! Action1() ) break;
        if ( ! Action2() ) break;
        if ( ! Action3() ) break;
        if ( ! Action4() ) break;
        if ( ! Action5() ) break;
        success = true;
    }
    return success;
}

instead of a more "correct"

private void Test()
{
    success = false;
    if ( Action1() )
    {
        if ( Action2() )
        {
            if ( Action3() )
            {
                if ( Action4() )
                {
                    if ( Action5() )
                    {
                        success = true;
                    }
                }
            }
        }
    }
    return success;
}

than sometimes I feel guilty, but sometimes I this it's not so bad at all. Anyone would like to comment? Many thanks in advance, Max

Answer 1

How about

success = Action1() || Action2() ||  ... ||  Action5();

Answer 2

 bool success = new Func<bool>[] { Action1, Action2, Action3, Action4, Action5 }.All(x => x());

The All extension method will keep calling the functions in order until one returns false or it runs out of functions and returns true. I think this solution, for the sake of readability, is the winner. Some might argue it being an overkill to instantiate an array for the sole purpose of readability, i.e. converting multiple lines of code into a single line :)

Those who don't know LINQ, lambda or C# will really disagree with my readability statement since the multiple if-statements can be understood by all programmers, regardless of language knowledge.

Answer 3

I'd write the following:

private boolean Test() {
    if (!Action1()) {
        return false;
    }
    if (!Action2()) {
        return false;
    }
    if (!Action3()) {
        return false;
    }
    if (!Action4()) {
        return false;
    }
    if (!Action5()) {
        return false;
    }
    return true;
}

Suggested reading: Flattening Arrow Code

Answer 4

Your functions are not equal.

• Variant 1 calls all your actions until at least one is false. That means it will loop and call Action1() again if all are true.
• Variant 2 will just do a logical and and call all actions only once.
• For them to be equal you would need a final break; after success = true; in Variant 1.

Assuming Variant 2 is correct, and Variant 1 just had a missing line, I would still not use the first variant. Ever. It is not following the KISS principle. And at first glance it will indicate something completely different, which is only good if you want to obfuscate your code.

Performance would be similar, so there is just no benefit from using Variant 1.

I however usually compress the if clauses into a single one, combining them with logical and.