Is it bad practice to use return inside a void method?

Question

Imagine the following code:

void DoThis()
{
    if (!isValid) return;

    DoThat();
}

void DoThat() {
    Console.WriteLine("DoThat()");
}

Is it OK to use a return inside a void method? Does it have any performance penalty? Or it would be better to write a code like this:

void DoThis()
{
    if (isValid)
    {
        DoThat();
    }
}

Answer 1

A return in a void method is not bad, is a common practice to invert if statements to reduce nesting.

And having less nesting on your methods improves code readability and maintainability.

Actually if you have a void method without any return statement, the compiler will always generate a ret instruction at the end of it.

Answer 2

There is another great reason for using guards (as opposed to nested code): If another programmer adds code to your function, they are working in a safer environment.

Consider:

void MyFunc(object obj)
{
    if (obj != null)
    {
        obj.DoSomething();
    }
}

versus:

void MyFunc(object obj)
{
    if (obj == null)
        return;

    obj.DoSomething();
}

Now, imagine another programmer adds the line: obj.DoSomethingElse();

void MyFunc(object obj)
{
    if (obj != null)
    {
        obj.DoSomething();
    }

    obj.DoSomethingElse();
}

void MyFunc(object obj)
{
    if (obj == null)
        return;

    obj.DoSomething();
    obj.DoSomethingElse();
}

Obviously this is a simplistic case, but the programmer has added a crash to the program in the first (nested code) instance. In the second example (early-exit with guards), once you get past the guard, your code is safe from unintentional use of a null reference.

Sure, a great programmer doesn't make mistakes like this (often). But prevention is better than cure - we can write the code in a way that eliminates this potential source of errors entirely. Nesting adds complexity, so best practices recommend refactoring code to reduce nesting.

Answer 3

Bad practice??? No way. In fact, it is always better to handle validations by returning from the method at the earliest if validations fail. Else it would result in huge amount of nested ifs & elses. Terminating early improves code readability.

Also check the responses on a similar question: Should I use return/continue statement instead of if-else?

Answer 4

It's not bad practice (for all reasons already stated). However, the more returns you have in a method, the more likely it should be split into smaller logical methods.

Answer 5

The first example is using a guard statement. From Wikipedia:

In computer programming, a guard is a boolean expression that must evaluate to true if the program execution is to continue in the branch in question.

I think having a bunch of guards at the top of a method is a perfectly understandable way to program. It is basically saying "do not execute this method if any of these are true".

So in general it would like this:

void DoThis()
{
  if (guard1) return;
  if (guard2) return;
  ...
  if (guardN) return;

  DoThat();
}

I think that's a lot more readable then:

void DoThis()
{
  if (guard1 && guard2 && guard3)
  {
    DoThat();
  }
}

Answer 6

There is no performance penalty, however the second piece of code is more readable and hence easier to maintain.

Answer 7

In this case, your second example is better code, but that has nothing to do with returning from a void function, it's simply because the second code is more direct. But returning from a void function is entirely fine.

Answer 8

It's perfectly okay and no 'performance penalty', but never ever write an 'if' statement without brackets.

Always

if( foo ){
    return;
}

It's way more readable; and you'll never accidentally assume that some parts of the code are within that statement when they're not.

Answer 9

I'm going to disagree with all you young whippersnappers on this one.

Using return in the middle of a method, void or otherwise, is very bad practice, for reasons that were articulated quite clearly, nearly forty years ago, by the late Edsger W. Dijkstra, starting in the well-known "GOTO Statement Considered Harmful", and continuing in "Structured Programming", by Dahl, Dijkstra, and Hoare.

The basic rule is that every control structure, and every module, should have exactly one entry and one exit. An explicit return in the middle of the module breaks that rule, and makes it much harder to reason about the state of the program, which in turn makes it much harder to say whether the program is correct or not (which is a much stronger property than "whether it appears to work or not").

"GOTO Statement Considered Harmful" and "Structured Programming" kicked off the "Structured Programming" revolution of the 1970s. Those two pieces are the reasons we have if-then-else, while-do, and other explicit control constructs today, and why GOTO statements in high-level languages are on the Endangered Species list. (My personal opinion is that they need to be on the Extinct Species list.)

It is worth noting that the Message Flow Modulator, the first piece of military software that EVER passed acceptance testing on the first try, with no deviations, waivers, or "yeah, but" verbiage, was written in a language that did not even have a GOTO statement.

It is also worth mentioning that Nicklaus Wirth changed the semantics of the RETURN statement in Oberon-07, the latest version of the Oberon programming language, making it a trailing piece of the declaration of a typed procedure (i.e., function), rather than an executable statement in the body of the function. His explication of the change said that he did it precisely because the previous form WAS a violation of the one-exit principle of Structured Programming.

Answer 10

While using guards, make sure you follow certain guidelines to not confuse readers.

• the function does one thing
• guards are only introduced as the first logic in the function
• the unnested part contains the function's core intent

Example

// guards point you to the core intent
void Remove(RayCastResult rayHit){

  if(rayHit== RayCastResult.Empty)
    return
    ;
  rayHit.Collider.Parent.Remove();
}

// no guards needed: function split into multiple cases
int WonOrLostMoney(int flaw)=>
  flaw==0 ? 100 :
  flaw<10 ? 30 :
  flaw<20 ? 0 :
  -20
;

Answer 11

Throw exception instead of returning nothing when object is null etc.

Your method expects object to be not null and is not the case so you should throw exception and let caller handle that.

But early return is not bad practice otherwise.