Which syntax is better for return value?

Question

I've been doing a massive code review and one pattern I notice all over the place is this:

public bool MethodName()
{
    bool returnValue = false;
    if (expression)
    {
        // do something
        returnValue = MethodCall();
    }
    else
    {
        // do something else
        returnValue = Expression;
    }

    return returnValue;
}

This is not how I would have done this I would have just returned the value when I knew what it was. which of these two patterns is more correct?

I stress that the logic always seems to be structured such that the return value is assigned in one plave only and no code is executed after it's assigned.

Answer 1

I would have used ternary, to reduce control structures...

return expression ? MethodCall() : Expression;

Answer 2

There is a lengthy discussion on this topic here.

Answer 3

They both accomplish the same task. Some say that a method should only have one entry and one exit point.

Answer 4

Some learning institutes and books advocate the single return practice.

Whether it's better or not is subjective.

Answer 5

I use this, too. The idea is that resources can be freed in the normal flow of the program. If you jump out of a method at 20 different places, and you need to call cleanUp() before, you'll have to add yet another cleanup method 20 times (or refactor everything)

Answer 6

I suspect I will be in the minority but I like the style presented in the example. It is easy to add a log statement and set a breakpoint, IMO. Plus, when used in a consistent way, it seems easier to "pattern match" than having multiple returns.

I'm not sure there is a "correct" answer on this, however.

Answer 7

I guess that the coder has taken the design of defining an object toReturn at the top of the method (e.g., List<Foo> toReturn = new ArrayList<Foo>();) and then populating it during the method call, and somehow decided to apply it to a boolean return type, which is odd.

Could also be a side effect of a coding standard that states that you can't return in the middle of a method body, only at the end.

Answer 8

Even if no code is executed after the return value is assigned now it does not mean that some code will not have to be added later.

It's not the smallest piece of code which could be used but it is very refactoring-friendly.

Answer 9

A lot of people recommend having only one exit point from your methods. The pattern you describe above follows that recommendation.

The main gist of that recommendation is that if ou have to cleanup some memory or state before returning from the method, it's better to have that code in one place only. having multiple exit points leads to either duplication of cleanup code or potential problems due to missing cleanup code at one or more of the exit points.

Of course, if your method is couple of lines long, or doesn't need any cleanup, you could have multiple returns.

Answer 10

That looks like a part of a bad OOP design. Perhaps it should be refactored on the higher level than inside of a single method.

Otherwise, I prefer using a ternary operator, like this:

return expression ? MethodCall() : Expression;

It is shorter and more readable.

Answer 11

Delphi forces this pattern by automatically creating a variable called "Result" which will be of the function's return type. Whatever "Result" is when the function exits, is your return value. So there's no "return" keyword at all.

function MethodName : boolean;
begin
  Result := False;
  if Expression then begin
    //do something
    Result := MethodCall;
  end
  else begin
    //do something else
    Result := Expression;
  end;

  //possibly more code
end;

Answer 12

The pattern used is verbose - but it's also easier to debug if you want to know the return value without opening the Registers window and checking EAX.

Answer 13

Return from a method right away in any of these situations:

1. You've found a boundary condition and need to return a unique or sentinel value: if (node.next = null) return NO_VALUE_FOUND;
2. A required value/state is false, so the rest of the method does not apply (aka a guard clause). E.g.: if (listeners == null) return null;
3. The method's purpose is to find and return a specific value, e.g.: if (nodes[i].value == searchValue) return i;
4. You're in a clause which returns a unique value from the method not used elsewhere in the method: if (userNameFromDb.equals(SUPER_USER)) return getSuperUserAccount();

Otherwise, it is useful to have only one return statement so that it's easier to add debug logging, resource cleanup and follow the logic. I try to handle all the above 4 cases first, if they apply, then declare a variable named result(s) as late as possible and assign values to that as needed.