tags:

views:

743

answers:

9

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();
    }
}
+1  A: 

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

Russell
Russell i disagree with your opinion but you shouldn't have been down-voted for it. +1 to even it out. Btw, i believe that a boolean test and return on a single line followed by a blank line is a clear indication of what's going on. e.g. Rodrigo's first example.
Paul Sasik
I disagree with this. Increasing nesting does not improve readability. The first piece of code is using a "guard" statement, which is a perfectly understandable pattern.
cdmckay
I disagree as well. Guard clauses which bail out of a function early are generally considered a Good Thing nowadays in helping a reader to understand the implementation.
Pete Hodgson
+34  A: 

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.

CMS
Short, sweet, and correct!
Paperino
+1  A: 

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.

Noon Silk
readable is subjective. imho, anything added to code that is unnecessary makes it less readable... (I have to read more, and then I wonder why it's there and waste time trying to make sure I'm not missing something) ...but that's my subjective opinion
Charles Bretana
The better reason to always include the braces is less about readability and more about safety. Without the braces it is all to easy for some one later to fix a bug that requires additional statements as part of the if, not pay close enough attention and add them without also adding the braces. By always including the braces, this risk is eliminated.
Scott Dorman
Silky, please press enter before your `{`. This lines up your `{` with your `}` in the same column, which greatly aids readability (much easier to find corresponding open/close braces).
Imagist
@Imagist I'll leave that to personal preference; and it's done the way I prefer :)
Noon Silk
@Scott I add braces but not for preventing bugs (I do it for convenience since I often end up adding logging commands, comments, or command line output for debugging purposes). What makes you so sure that `if(foo) bar(); baz();` is any more common a mistake than `if(foo) { bar(); } baz();`? I don't think I've ever fixed a bug caused by either situation, so the bug concern seems minor here.
Imagist
@Imagist; re bugs, it happens when tabbing isn't automatic. You can prese enter after the line without braces, and due to indendation assume you are within that 'if' statement. But accidentally, you aren't.Always add the brackets. It doesn't hurt, and keeps things simple.
Noon Silk
+4  A: 

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?

Rashmi Pandit
+1  A: 

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.

Imagist
+1  A: 

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.

Mike Hall
A: 

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.

John R. Strohm
How do Exceptions fit here?
nairdaen
@John: we got over the Dykstra injunction about multiple returns just about the time we got over Pascal (most of us, anyway).
John Saunders
Cases where multiple returns are required are frequently signs that a method is trying to do too much and should be pared down. I'm not going to go as far as John with this, and a return statement as part of parameter validation may be a reasonable exception, but I get where the idea is coming from.
kyoryu
@nairdaen: There is still controversy over exceptions in that quarter. My guideline is this: If the system under development MUST fix the problem that caused the original exceptional condition, and I don't mind pissing off the guy who will have to write that code, I'll throw an exception.Then I get yelled at in a meeting, because the guy didn't bother to catch the exception, and the app crashed in testing, and I explain WHY he has to fix the problem, and things settle down again.
John R. Strohm
There is a big difference between guard statements and gotos. The evil of gotos is that they can jump anywhere, so can be very confusing to unravel and remember. Guard statements are the exact opposite - they provide a gated entry to a method, after which you know that you are working in a "safe" environment, reducing the number of things you have to consider as you write the rest of the code (e.g. "I know this pointer will never be null, so I don't need to handle that case throughout the code").
Jason Williams
@Jason: The original question was not specifically about guard statements, but about random return statements in the middle of a method. The example given example appeard to be a guard.The key issue is that, at the return site, you want to be able to reason about what the method did or didn't do, and random returns make that harder, for EXACTLY the same reasons that random GOTOs make it harder. See: Dijkstra, "GOTO Statement Considered Harmful".On the syntax side, cdmckay gave, in another answer, his preferred syntax for guards; I disagree with his opinion of which form is more readable.
John R. Strohm
+1 for Dijkstra!
Matt Nizol
@John: I find much less problem with multiple returns now, than we did in the days of structured programming (before OO). In good code, I don't need to do much reasoning about what the method did or didn't do - I assume it did what it's named for, or that it would have thrown an exception, or sometimes returned `false`.
John Saunders
Undeniably clever though Dijkstra is, I think that one should be careful taking the advice of somebody who only uses his computer for "e-mail and for browsing the World Wide Web" because it's entirely possible that they might fall out of touch with real-world development.
spender
+1  A: 

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();
  }
}
cdmckay
+2  A: 

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.

Jason Williams
Yes, but on the other hand, several layers of nesting, with their conditions, make the code even more prone to bugs, the logic more difficult to track and - more importantly - harder to debug. Flat functions are lesser evil, IMO.
Skrim
I'm arguing in *favour* of reduced nesting! :-)
Jason Williams