If a function contains only a switch, is it bad practise to replace the break; statements with return; statements?

Question

Lets say I have a function that takes an argument, does some action based on the value of that argument and returns false if there is no action for that value. (pseudo-code):

bool executeSomeAction(someValue) {
    switch (someValue) {
    case shouldDoAction1:
        action1();
        break;

    case shouldDoAction2:
        action2();
        break;

    default:
        return false;
    }
    return true;
}

Is it bad practise to write it like this:

bool executeSomeAction(someValue) {
    switch (someValue) {
    case shouldDoAction1:
        action1();
        return true;

    case shouldDoAction2:
        action2();
        return true;

    default:
        return false;
    }
}

Edit: I don't see how this is a duplicate of the linked question, I'm asking about the best practise of writing a function with a single switch statement, I'm not at all asking about using a single or multiple return statements.

Answer 1

It's definitely not bad practice. I use the direct return pattern quite a lot for factory/strategy methods, where different classes implement the same interface and a specific implementation is retrieved based on some strategy.

Not using direct return, having to use the break statement and perhaps having a single point of return from a function may leave the developer reading your code wondering, whether a variable is or is not altered at any further point of the function. Should you return immediately the conclusion is very simple: the function ends.

If you are at a place where you can safely return from a function, do so.

I would choose the option 2 over 1, or even better, change the action1 and action2 functions to return boolean values themselves and do direct return action1() or return action2() respectively.

bool executeSomeAction(someValue)
{
    switch (someValue)
    {
    case shouldDoAction1:
        return action1();
    case shouldDoAction2:
        return action2();
    default:
        return false;
    }
}

Answer 2

I don't believe it is bad practice to write such code. On the contrary, I believe that it is very good practice. If the execution reaches a point where the function has nothing more to do, returning immediately is the best and most obvious thing to do. If I read return, I know immediately what is going on. If I read break, I first have to check where the control flow jumps next.

That said, I'd be consistent. Either each and every case (including the default) should contain a return or none. The mixing as in your first snippet is even less readable, I think. Since a switch is already hard to follow, I try to keep it as simple (least surprises for the reader) as possible.

Answer 3

I would definitely go with the second option, based on the KISS principle. There is absolutely no gain in breaking, and then returning in the next instruction.

The same principle applies in the following snippet which I see more often than I would like to when doing code review:

if (somecondition)
     return true;
else
     return false;

If there is no logging or anything else involved, then just replace the above snippet with:

return somecondition;

Answer 4

Either way is okay. Personally, I usually wouldn't return() in the middle of a function's code. Rather, I usually have a block of code at the end of each function labelled "end_of_job:" that does any necessary housekeeping, fprintf()'s any debugging messages (under a guard), etc. Even if your function doesn't currently need any of this, you may want or need it later. And maybe your return(value); will change sometime down the road -- you don't want to miss editing any in-the-middle return()'s. Similarly, other people maintaining your code won't have opportunities to overlook any such "hidden" return()'s. Instead, anyplace where return() is needed, I just have "goto end_of_job;" instead. Here's a mocked-up example...

/* ---
 * end-of-job
 * ------------- */
end_of_job:
  /* --- eoj housekeeping --- */
  if ( temp_buffer != NULL ) free(temp_buffer);
  /* --- debugging --- */
  if ( msglevel>=99 && msgfp!=NULL ) {
    fprintf(msgfp,"func_name> this=%d, that=%f, etc\n",
    this,that,etc); fflush(msgfp); }
  /* --- back to caller --- */
  return(whatever);
} /* --- end-of-function func_name --- */