What is the perfect way to write "return" in a method

Question

I don't like methods have several return lines. So I created a return value with string result - and in every condition I write result = something...

But when I write "try-catch" mechanism, I have to set public string result. Because, if I return a result in try, compiler will launch error, and says not all codes have return value. If I write result = string.Empty to end of the method, resharper says, it's not reachable code. So, here an example, and here is my question;

"What is the perfect way to write "return" in a method ?"

    public static string PingThatAddress(string hostAddress)
    {
        try
        {
            Ping ping = new Ping();
            PingReply pingreply = ping.Send(hostAddress);

            string result;
            if (pingreply != null && pingreply.Status.ToString() != "TimedOut")
            {
                result = "Address: " + pingreply.Address + "\r"
                     + "Roundtrip Time: " + pingreply.RoundtripTime + "\r"
                     + "TTL (Time To Live): " + pingreply.Options.Ttl + "\r"
                     + "Buffer Size: " + pingreply.Buffer.Length + "\r";
            }
            else
            {
                result = string.Empty;
            }

            return result;
        }
        catch (Exception pingError)
        {
            Debug.Fail(pingError.Message + " " + pingError);
        }
        //compiler error: THERE IS NO RETURN VALUE here?
    }

"I don't like methods have several return lines" - why? It is one of the best way to simplify a method. — Oded, Jan 15 '12 at 21:58
Which exceptions are you expecting, and why are you catching *and handling* other exceptions as well? — , Jan 15 '12 at 22:15
If you are having doubts on using multiple return statements, have a look at how [stackoverflow votes on this matter](http://stackoverflow.com/questions/36707/should-a-function-have-only-one-return-statement) — Maarten Bodewes, Jan 16 '12 at 01:33
@Oded I also dislike multiple returns in a method. A return is nothing else then a "goto" hidden in a nice cloth. — Offler, May 22 '13 at 12:53

Andrew Barber · Accepted Answer · 2012-01-15T22:08:35.807

You could do it like this instead:

public static string PingThatAddress(string hostAddress)
{
    string result = string.Empty;
    try
    {
        Ping ping = new Ping();
        PingReply pingreply = ping.Send(hostAddress);

        if (pingreply != null && pingreply.Status.ToString() != "TimedOut")
        {
            result = "Address: " + pingreply.Address + "\r"
                 + "Roundtrip Time: " + pingreply.RoundtripTime + "\r"
                 + "TTL (Time To Live): " + pingreply.Options.Ttl + "\r"
                 + "Buffer Size: " + pingreply.Buffer.Length + "\r";
        }

    }
    catch (Exception pingError)
    {
        Debug.Fail(pingError.Message + " " + pingError);
    }
    return result;
}

Then just make sure result is set to something that makes sense in the case of an exception.

If you are trying to stick with what Resharper is warning you about, do it this way:

public static string PingThatAddress(string hostAddress)
{
    try
    {
        Ping ping = new Ping();
        PingReply pingreply = ping.Send(hostAddress);

        string result = string.Empty;
        if (pingreply != null && pingreply.Status.ToString() != "TimedOut")
        {
            result = "Address: " + pingreply.Address + "\r"
                 + "Roundtrip Time: " + pingreply.RoundtripTime + "\r"
                 + "TTL (Time To Live): " + pingreply.Options.Ttl + "\r"
                 + "Buffer Size: " + pingreply.Buffer.Length + "\r";
        }
        return result;

    }
    catch (Exception pingError)
    {
        Debug.Fail(pingError.Message + " " + pingError);
    }
    return string.Empty;
}

You can not have things both ways here: Your artificial standard not to have multiple return statements is probably what is causing you to have trouble with Resharper.

Thank you very much for your answer. But now, resharper says; "Move decleration to closer usage". I ask this question for what is the perfect way to do it in coding standarts. I don't want to ignore resharper recommendition in any case. — Lost_In_Library, Jan 15 '12 at 22:03
If I write result = string.Empty to end of the method, resharper says, it's not reachable code. Damn resharper =) But I think there is best way to do it. I don't know. It has to be a best way... — Lost_In_Library, Jan 15 '12 at 22:08
@LostInLib I didn't say `result = string.Empty`; I said `return string.Empty`. Check out my new answer — Andrew Barber, Jan 15 '12 at 22:10
I take the first way as an answer. I think this is the perfect way to do it. And thank you very much Andrew... — Lost_In_Library, Jan 15 '12 at 22:14

score 2 · Answer 2 · answered Jan 15 '12 at 22:33

Some of the suggestions, including the first part of the accepted answer, and the original question have a strong potential to return incorrect results especially in the event of an exception.

The problem, and we have seen this happen numerous times, is that when you have a single return value, minor changes to the method's logic will invariably result in a path through the code that was not anticipated by the original method writer and will result in either the return variable incorrectly being set multiple times in the method or not set at all.

There are definitely times when you have to collect a value for returning to the caller and then perform some additional tasks in the method subsequent to that, but by and large that should be the exception rather than the rule.

After tracking down far too many bugs that were introduced by the desire to have a single return value, our development standards now dictate that return will be used unless absolutely necessary and the reason for not doing so must be thoroughly documented in the code along with warnings for subsequent modifiers.

The added benefit of this approach is that if the logic of the method is modified in such a way that the new code path causes a "hole" in the return logic, you will be automatically notified about this by the compiler. Using a single return value requires the developer to visually inspect every possible code path in order to verify that nothing was missed.

Finally, rather than having a return value outside of the exception, we require that the appropriate default value be returned from within the exception handler. This way it is crystal clear as to what will happen in the event of an exception.

So in our environment, your code would be:

public static string PingThatAddress(string hostAddress)
{
    try
    {
        Ping ping = new Ping();
        PingReply pingreply = ping.Send(hostAddress);

        if (pingreply != null && pingreply.Status.ToString() != "TimedOut")
        {
            return "Address: " + pingreply.Address + "\r"
                 + "Roundtrip Time: " + pingreply.RoundtripTime + "\r"
                 + "TTL (Time To Live): " + pingreply.Options.Ttl + "\r"
                 + "Buffer Size: " + pingreply.Buffer.Length + "\r";
        }
        else
        {
            return string.Empty;
        }
    }
    catch (Exception pingError)
    {
        Debug.Fail(pingError.Message + " " + pingError);
        return string.Empty;
    }
}

Thank you very much your answer. You are right, but I try to do everything what resharper recommend to me ( not always, but I try ). And resharper says to your code; "else" block and "return string.Empty;" in catch block are unreachable codes. - I click your answer as useful. — Lost_In_Library, Jan 15 '12 at 22:46
That is a good general approach (following resharper's instructions), but in this case, there appears to be a bug in resharper. First of all, in release mode, Debug.Fail will never be executed and thus the return will be reached. Second, in debug mode, all Debug.Fail does is display a message and let's the user decide what to do. At least one of the offered choices is to ignore the exception and carry on, in which case the return will also be reached. Thus, there is a bug in the resharper analysis of this code. — competent_tech, Jan 15 '12 at 22:55
In "Andrew Barber"'s first answer, Debug.Fail will never be executed in compiled application(not debug, compiled in release mode). But program will continue with exiting catch block and read the following statement "return result;"( result is string.Empty ). Am I right? So in which in case? & what is the bug will be appear? — Lost_In_Library, Jan 15 '12 at 23:07
In the method as it is designed in his answer there is no problem. However, let's say that someone introduces some new code after result is set that causes an exception or even worse, someone adds another line to the code area where result is set to add some more content to result and that code causes an exception. In either of these cases, result will contain a value that will incorrectly be returned to the caller. — competent_tech, Jan 15 '12 at 23:17

Maarten Bodewes · Answer 3 · 2012-01-16T01:28:59.153

So you are claiming that the if statement (or other program flow statement) which is helping you get to that exit point, isn't actually an exit point by itself? The only difference between that control statement and the return statement is that the return statement is actually more visible, more readable, more maintainable and less error prone. Also, as the return statement will work on any level, you are likely to repeat the control statement an x number of times.

The only reason to want to have the return statement at the end is resource handling - getting rid of the resources you claimed at the start. This is much more common in C/C++ (and even in C++ it is getting less troublesome to do without). In Java, you can rely on the exception mechanism and the finally statement to handle resources as they should be (and in Java 7, the "try with resources" feature).

There are other reasons to choose for the return statement as well. One is the ability to mark variables final: as the compiler knows that return (or a throw for that matter) is the end of the method, you can more easily insert final variables. Final variables - once you get used to them - really make it easy to read code and help you avoid making mistakes.

  boolean someErrorCondition = true;
  final int setSomething;
  for (int i = 0; i < 8; i++) {
      if (i == 7) {
          setSomething = 11;
          break;
      }

      if (someErrorCondition) {
          return;
      }
  }

This won't work with your approach because the compiler will complain about the final variable not being set (which was the whole idea, not getting into invalid states).

Multitudes of excellent developers and language designers have choosen in favour of multiple return statements. I would urgently advise anyone against going in the other direction.

sorry, that was Java in the example, but you get the drift – Maarten Bodewes Jan 16 '12 at 00:42 — Maarten Bodewes, Jan 16 '12 at 00:42

score 0 · Answer 4 · answered Sep 26 '13 at 08:18

Multiple return statements make it easier to follow your code. In this way you immediately see different method return points. As an opposite, one returning 'result' field forces you to find where the field is used after it has been changed. It makes it harder to follow the natural flow of the method. But anyway, usually the desicion is more about style preferences. Make sure you do not mix two approaches.

score 0 · Answer 5 · answered Jan 15 '12 at 21:59

0

In case of an exception you don't return a value.

answered Jan 15 '12 at 21:59

rekire

47,260
30
167
264

1

The OP is catching and handling the exception; it *may* or *may not* be appropriate to return a value here. – Andrew Barber Jan 15 '12 at 22:00
Thank you very for your answer. But I don't get it. Why I shouldn't return a value in try-catch block ? – Lost_In_Library Jan 15 '12 at 22:04
In my opinion LostInLib could write a return statement in the catch block or use your code. While the enhancment of your code is a single return statment. – rekire Jan 15 '12 at 22:04
@rekire My code was to prevent a compilation error, and it did mostly amount to adding a single return statement... but your "in my opinion" is exactly the same thing: adding a single return statement. My enhancements to the code, though, were initializing the `result` variable, and then removing an unneeded `else` block. – Andrew Barber Jan 15 '12 at 22:33
@AndrewBarber devs should *try* and not have any exceptions thrown in their code when it is running properly. The only reason for catching an exception and returning would be if an external API returns an exception which you cannot reasonably put into another exception - it's a "good" scenario for your application. Fortunately, that is exectly what is happening in the code of the OP, LostInLib. – Maarten Bodewes Jan 16 '12 at 00:41

What is the perfect way to write "return" in a method

5 Answers5

Linked