Refactoring method with many conditional return statements

Question

Hi,

I have a method for validation that has many conditional statements. Basically it goes

If Check1 = false 
  return false
If Check2 = false
  return false
etc

FxCop complains that the cyclomatic complexity is too high. I know that it is not best practice to have return statements in the middle of functions, but at the same time the only alternative I see is an ugly list of If-else statements. What is the best way to approach this?

Thanks in advance.

Answer 1

I disagree with your claim that it's not best practice to have return statements in the middle of methods. The lengths some people go to in order to have a single return statement is crazy - go with whatever produces the most readable code. Sometimes that will be a single point of return, but often I find there's an "early out" - and it's better to have that return than to introduce more nesting to the code with an if for the alternative path. I like methods that don't end up indented too far, as a rule of thumb :)

Having said that, is the method really nothing but checks? Are the checks independent? What variables do they require? Can you group them into smaller methods representing "areas" of the checks you're performing?

Answer 2

Using your example:

return (Check1 == false)   ||   (Check2 == false)   [ || etc ]

Answer 3

For your particular case, it seems you could probably use a loop... assuming this came out of an event handler in a Form or something:

For Each ctrl As Control In Me.Controls

    Dim check As CheckBox = TryCast(ctrl, CheckBox)

    If check IsNot Nothing AndAlso check.Checked = False Then
        Return False
    End If

Next

Return True

Edit: Upon further review, I realize that this was based off of psuedocode and you weren't actually use check boxes. However, I think the same methodology applies. You could very easily refactor your rules into a separate class which implements a custom interface (IRule or similar), or have a list of delegates which return true/false which you would iterate over as part of your check pattern.

Answer 4

If you have several checks in a row and don't need to preserve short-circuiting, you might try something like this:

        bool isValid = true;
        isValid &= Condition1;
        isValid &= Condition2;
        isValid &= Condition3;
        if (!isValid)
            return false;

If you do need to preserve short-circuiting, you can consolidate the conditions into a large if statement. If there are a lot of conditions, however, I would prefer the original code, with multiple returns, as a single large 'if' could get a bit ugly.

Note that, in the latter case, you're merely side-stepping the metric, as the logical branching is actually the same. If the condition checks do not have side effects (and I really hope they don't), then I believe high cyclomatic complexity is a false alarm, as the multiple returns are really just a more readable way of expressing complex boolean validation logic.

This is only true, however, if the conditions are all in series. If they are spread throughout the code, with meaningful processing happening between checks, then the metric is indicating real complexity (more branches that must be tested). In this case, you might consider whether the method can be logically broken into smaller, individually testable pieces.