A rule of thumb when coding large nested function blocks

Question

I have been coding C # code for some time now and overall have a good understanding of the rule of thumb for coding standards. My colleagues have recently encouraged a results-based approach to coding function blocks rather than logic blocks and are looking for 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, not nesting. It has been suggested that this approach is easier to read, especially if the result requires several levels of nesting, but I prefer to use Curlys Law and refactoring methods and functions where the nesting becomes 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 seems wrong to me, as I would suggest that the method should be rewritten as ..

  private void MethodOne() { // first layer if (ValidationRuleOne() == true) { // second layer if (ValidationRuleTwo() == true) { // third layer if (ValidationRuleThree() == true) { } } } } 

If the embedding becomes too complicated, would I suggest that the structure of the method / function be rethought to combine the logical groups of functions together into additional methods or functions?

Any thoughts were greatly appreciated.

Respectfully,

Tim

Answer 1

 if (ValidationRuleOne() == true) { // second layer if (ValidationRuleTwo() == true) { // third layer if (ValidationRuleThree() == true) 

may be:

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

in my opinion, much easier to read.

and if for some reason you need each validation rule:

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

(and you do not need to do if (validationRule () == true), since validation returns a boolean, you do not need to compare it with one in the conditional expression).

Answer 2

I personally like to use security offers.

 if (!ValidationRuleOne()) return; if (!ValidationRuleTwo()) return; if (!ValidationRuleThree()) return; 

Answer 3

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

I agree with Kevin's suggestion to use && if you have nothing more to do ... I assume that in real life you have different code in the if and else blocks. If you do not, you should simply use && .

Also, in terms of style:

• Do not compare booleans with true or false:

   if (ValidationRuleOne() == true) 

  usually should be

   if (ValidationRuleOne()) 

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

• If a variable is set in each path (as in your first, if / else), do not assign it immediately ... this value will be discarded.

Answer 4

I would prefer:

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

Answer 5

Another alternative might be to use:

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

This has several advantages.

• You can do extra work between checking various validation rules.
• .. but you can still easily check more things on one line if you don't need extra work.
• You avoid backing down code far to the right (what happens with nested if s)

The only limitation is that you may need to move this to a separate method so that you can use return to exit the method if the test fails.

Answer 6

Staying in the functional world - if you can work with lambda expressions in C #, this is exactly what you can use to continue, in your case the continuation for each verification procedure will be performed only if the verification has passed.

  ValidationRuleOne(() => ValidationRuleTwo(() => ValidationRuleThree())); public void ValidationRuleOne(Action continuation) { //... if(validationSuccessful && continuation!=null) continuation(); } 

Answer 7

I experienced both, and I think both are fine. I think it really comes down to personal preferences and ease of use / understanding.

Answer 8

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

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

 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(); } }