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?
    }

Answer 1

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.

Answer 2

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.

Answer 3

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;
    }
}

Answer 4

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.

Answer 5

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