rule of thumb when coding large nested functional blocks

Question

Hello All, I’ve been coding in c# for a while now and generally have a good idea about the rule of thumb for coding standards. I’ve been recently encourage by my colleges to adopt a results based approach to coding functional blocks rather than nesting blocks of logic and am seeking your advice. Below is an example of what I’m talking about, where the result of one situation is used to determine the code path rather than nesting. It’s been suggested that this approach is easier to read, especially if the result would require several layers of nesting, however I prefer to uses Curly’s Law and refactor methods and functions where nesting becomes to deep.

private void MethodOne()
    {

        bool CarryOn = false;

        // first layer
        if (ValidationRuleOne() == true)
        {
            CarryOn = true;

        } else {

            CarryOn = false;
        }

        // second layer
        if (CarryOn) 
        {
            CarryOn = ValidationRuleTwo();

        } else {

            CarryOn = false;
        }

        // third layer
        if (CarryOn)
        {
            CarryOn = ValidationRuleThree();

        } else
        {

            CarryOn = false;
        }

    }

This approach just seems wrong to me as I would suggest that the method should be rewriten as ..

        private void MethodOne()
    {



        // first layer
        if (ValidationRuleOne() == true)
        {

            // second layer
            if (ValidationRuleTwo() == true)
            {

                // third layer
                if (ValidationRuleThree() == true)
                {


                }

            }
        }

    }

If the nesting becomes too complex then I would suggest that the method/function structure needs to be rethought to group logical groups of functionality together into additional methods or functions are required?

Any thoughts greatly appreciated.

Regards,

Tim

Answer 1

if (ValidationRuleOne() == true)
    {

        // second layer
        if (ValidationRuleTwo() == true)
        {

            // third layer
            if (ValidationRuleThree() == true)

can be:

if(ValidationRuleOne() && ValidationRuleTwo() && ValidationRuleThree())
{
...
}

in my opinion much easier to read.

and if for some reason you need each validation rule to fire:

if(ValidationRuleOne() & ValidationRuleTwo() & ValidationRuleThree())
{...} //note the & vs &&

(and you don't need to do if(validationRule() == true), since validation returns a boolean, you don't need to compare it to one in a conditional statement).

Answer 2

Any reason why you're not just returning from the else block instead of setting "CarryOn" to false?

I agree with Kevin's suggestion of using && if you don't have anything else to do... I'm assuming that in real life you have other code in the if and else blocks. If you don't you absolutely should just use &&.

Also, in terms of style:

• Don't compare Boolean values with true or false:

  if (ValidationRuleOne() == true)
  

  should usually be

  if (ValidationRuleOne())
  

• Name local variables in camelCase, so carryOn instead of CarryOn.

• If a variable is going to be set in every path (as in your first if/else), don't assign it immediately... that value is going to be discarded.

Answer 3

I have experienced both, and I believe both are ok. I think, its really boils down to personal preferences and ease of use/understanding.

Answer 4

I would prefer:

private bool IsValid()
{
    return ValidationRuleOne() && ValidationRuleTwo() && ValidationRuleThree();
}

Answer 5

I personally like using guard clauses.

if (!ValidationRuleOne()) 
    return;

if (!ValidationRuleTwo())
    return;

if (!ValidationRuleThree()) 
    return;

Answer 6

Another alternative would be to use:

if (!ValidationRuleOne()) return;
// Do some work
if (!ValidationRuleTwo() || !ValidationRuleThree()) return;

// The main body

This has a couple of benefits

• You can do additional work between checking for various validation rules
• .. but you can still easily validate more things in a single line if you don't need additional work.
• You avoid indenting the code far to the right (which happens with nested ifs)

The only limitation is that you may need to move this into a separate method, so that you can use return to exit the method if validation doesn't hold.

Answer 7

Staying in the functional world - if you can work with lambda expressions in C#, this is exactly what you can use continuations for, in your case the continuation for each validation routine would only be executed if the validation passes.

  ValidationRuleOne(() => ValidationRuleTwo(() => ValidationRuleThree()));

public void ValidationRuleOne(Action continuation)
{
  //...

 if(validationSuccessful  && continuation!=null)
    continuation();
}

Answer 8

For the example as given, @Darin Dimitrov's solution is the best.

If there are other actions that are taken depending on the result of verification, the obvious solution is goto!

private void MethodOne() 
{ 

    bool isValid = false; 

    // first layer 
    isValid  = ValidationRuleOne();
    if (!isValid) { goto tagApplyValidationResult; }

    isValid  = ValidationRuleTwo();
    if (!isValid) { goto tagApplyValidationResult; }

    isValid  = ValidationRuleThree()

 tagApplyValidationResult:
    if (!isValid) { Whine(); }
    else          { DoTheHappyDance(); }
}