Best Practices: Returning from method in case block of switch statement?

Question

Which method of returning a bool from a switch statement is preferable? I understand this may be subjective, however I feel it is important to our profession to get input on best practices :).

public bool foo(param)
{
    switch (param)
    {
        case 1:
            if (something)
            {
                return true;
            }

            return false;    
        default:
            return false;
     }
}

- OR -

public bool foo(param)
{
    bool flag = false;

    switch (param)
    {
        case 1:
            if (something)
            {
                flag = true;
            }
            break;
        default:
            break;
     }

    return flag;
}

Answer 1

The difference here is between single point of return and multiple points of return.

Generally, single point of return code tends to do a lot of bookkeeping (temporary variables, checking of those variables in loops), and can get hairy as the logic gets more complex. I've seen code that goes to great lengths to use a while (flag) ... flag = false; pattern instead of a while (true) ... break; pattern, and it is not fun to read.

I prefer multiple points of return since they return as early as possible (no extra work done) and don't require any locals to help keep track of the current return value. Also, I don't find them any harder to read than single point of return code.

There's a good disucssion of this on c2 (SingleFunctionExitPoint).

I've found that keeping methods as "functional" as possible is a good thing (I say "functional" here because I'm typically doing C# which, outside of LINQ, is not considered functional). By "functional", I mean that I try to avoid mutating state as much as possible. Introducing more state in the form of locals or members makes things more difficult to understand since you now have to take into consideration their values and you have to consider how those variables can be set (from another method, from another class, ...).

In general, state is evil, but sometimes it's a necessary evil.

This particular issue has some interesting conversation here on SO as well.

Answer 2

I prefer the second method.
It is much more adaptable, you can for example add some work at the end of the function which will depend on flag

Also, if you only have one case, you may want to move to something other than a switch

Answer 3

I prefer the early return - but in general, I really try to avoid switch if at all possible unless it is choosing between at most 3 options. And I try to isolate the switch as much as possible, so that the method with the switch only determines the variable part - everything else is handled by the calling code.

IMO, the early return is easier to read as I don't need to 'read' further when I encounter a return.

Answer 4

This looks like a really elaborate ploy to avoid the && operator. Boolean expressions are coolio:

public bool foo(int param)
{
    return (param == 1) && something;
}

Answer 5

Given the two options, I say neither because they are nesting too far -- you should be calling a function that returns a boolean in your examples.

Given the question between single point of return and multiple point of return (and my previous point), I take the multiple point of return because you can just return the value your function for that case returned.

As an example:

public bool foo(param)
{
    switch (param)
    {
        case 1:
            return MethodForParamEqualOne();
        default:
            throw new ArgumentException("Param value=" + param + " is not supported.");
    }
    //If you're doing stuff here, the method is likely doing too much.
}

Eventually these switch statements get refactored out into method calls off objects (which will improve the terrible nesting switches give you). If you're real savvy, you may not even need the switch statement in your object factory (xml configs, reflection, database, ...).

Answer 6

It all depends on what I'm doing. If I do the case and then I'm done, I will return--why execute unnecessary code? If I want to do more work, then I set a flag or something. Use the right tool for the right job.

Answer 7

I would remove the switch statement altogether:

public bool foo(param)
{
   if(param != 1)
   {
    throw new ArgumentException("Param value=" + param + " is not supported.");
   }

   return MethodForParamEqualOne();
}

Answer 8

When it comes to returns and choice of conditional blocks it may be a good idea to always keep in the back of your head, to comment critical code. If you use

while(true)

don't be afraid to throw a

//WARNING + reason!

on top of the break code. Usually that's overkill most people will tell you , but as soon as it's gets a little bit more dicy with

for (int x = 0; /*external variable for condition*/ ; x ++)

I have found that helped me preventing code wrecking quite often when quickly trying to test small changes in a lot of code you actually don't have to read as much code anymore giving a small time bonus for others.

A bit of a peanut thing to be honest, but I just figure I'd share what has helped me, so it may help others.

bool a = false;

while(true)
{
   ReDraw();
   ReUpdate();       

   //WARNING! Critical breakpoint   <--- Edit , nobody likes a game you can't win :P
   a = CheckForWinGame();
   if (a) 
   { 
      break; 
   }
}

In this example I hope to show that the warning told me now in the blink of an eye, I can meddle with methods, but not with the last. It's not waterproof but it's quick