2

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.

  • 1
    If you need a switch, this could be a sign that a code needs refactoring. When it comes to these two options, I think that first one is better. – Mac70 Sep 28 '16 at 8:05
  • 3
    Um, a switch is a perfectly good structure when used properly. It is very useful if you want to explicitly list out all possibilities, but also have groups of results with the same answers. A switch doesn't mean it needs refactoring. – Nelson Sep 28 '16 at 8:44
  • 2
    @Nelson, my absolute pet hate in programming is sentences of the form "x is a perfectly good feature when used properly". I see folk apply it to singletons, goto, inheritance, mutability, switch etc etc etc. The problem is, in 99.999% of the time, it isn't used properly. So "don't use feature x" is actually excellent advice. It can then be safely ignored by the tiny group of folk who understand about edge cases where it is needed and only use it in those cases. And from experience those who says "x is a perfectly good feature when used properly" don't know when to use it properly. – David Arno Sep 28 '16 at 9:03
  • 3
    Whether one should use a switch or not is a whole other debate. Lets keep it related. – Kevin Sep 28 '16 at 9:16
  • Possible duplicate of Where did the notion of "one return only" come from? – gnat Sep 28 '16 at 10:05
3

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;
    }
}
  • I don't think I should directly return the results of action1 and action2 in my case, but that's certainly worth considering in other cases, I hadn't even thought about that. – Kevin Sep 28 '16 at 8:52
  • I would assume your functions currently return nothing? Or are you just ignoring what they return because they're not relevant to the operation? – Nelson Sep 28 '16 at 9:23
  • @Nelson I'm working on a phonegap plugin, the functions call a bridge to a native API, so it's all a bit more complicated than that – Kevin Sep 28 '16 at 9:47
1

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.

1

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;
  • if (somecondition == true) – 5gon12eder Sep 28 '16 at 9:12
  • The gain in breaking would be that if the function ever changes in a way that makes it impossible to return directly from within the case statements, there is less refactoring, making it less likely to introduce new bugs in the future. – Kevin Sep 28 '16 at 9:17
  • Then you are breaking YAGNI. Until a need arises, do not overengineer the code, unless you are absolutely, 100% certain that the code will change in the proximate future. – Vladimir Stokic Sep 28 '16 at 9:50
  • 1
    @5gon12eder I can understand adding == false, because it might make the code more readable, e.g. if (!somecondition) as opposed to if (somecondition == false). However, adding == true to the condition makes the code more cluttered to me, so personally I avoid doing that. – Vladimir Stokic Sep 28 '16 at 9:59
  • @VladimirStokic I wish there was a principle for not trying to shoehorn every damn principle in every project. Adhering to things like DRY, KISS, YAGNI, etc can have great advantages, but they aren't 100% universal and should definitely not be the baseline for all code in all projects. – Kevin Sep 28 '16 at 10:32
-5

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 --- */
  • 4
    This all sounds horrible... – Kevin Sep 28 '16 at 10:28
  • 1
    Please, John, don't do this anywhere in production. Learn a better way. – Andy Sep 28 '16 at 11:37
  • Usually msglevel=0, so nothing's displayed. Also, there's typically a corresponding fprintf at the entry point of each function, displaying all inout arguments, to the extent feasible. That way, when there's a bug, you bump msglevel, and immediately see the entire program flow, along with all function inputs/outputs. Usually (though not 100% always) way better than running the debugger and setting breakpoints. And most languages don't have multiple entry points for functions (Fortran being an exception). Multiple return points are typically a similarly less-than-ideal idea. – John Forkosh Sep 29 '16 at 7:18
  • Every time I feel tempted to add such “headings comments” indicating what the following block of code does next, I know that I should refactor. – 5gon12eder Sep 29 '16 at 17:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.