14
\$\begingroup\$

In the situation where I am looping through a collection looking for an item, is it better practice to break before I return or should I break and then return?

Return during loop:

foreach(string item in items)
{
    string[] split = item.Split('|');
    if(split[0] == searchFor)
        return split[1];
}
return null;

Break then return:

string result = null;
foreach(string item in items)
{
    string[] split = item.Split('|');
    if(split[0] == searchFor)
    {
        result = split[1];
        break;
    }
}
return result;
\$\endgroup\$

migrated from stackoverflow.com Apr 30 '11 at 6:07

This question came from our site for professional and enthusiast programmers.

  • 3
    \$\begingroup\$ return (from split in items.Select(item => item.Split('|')) where split[0] == searchFor select split[1]).FirstOrDefault(); \$\endgroup\$ – Otávio Décio Apr 29 '11 at 19:58
  • \$\begingroup\$ I like the second because it would be easier to add logging. \$\endgroup\$ – garykindel Apr 29 '11 at 20:11
  • \$\begingroup\$ 'Better' or 'Best' is quite subjective. \$\endgroup\$ – Tim Post Apr 30 '11 at 6:07
  • \$\begingroup\$ return (from item in items where item.StartsWith(searchFor + '|') select item.Split('|')[1]).FirstOrDefault(); is even shorter — but is it more readable? Doubtful. \$\endgroup\$ – Timwi May 4 '11 at 20:25

10 Answers 10

18
\$\begingroup\$

If you break, you need to introduce an additional variable. In my opinion, it's more clear to return as soon as possible from any function.

\$\endgroup\$
  • 1
    \$\begingroup\$ Unless of course you have objects which need to be cleaned up before returning. \$\endgroup\$ – McMinton Apr 29 '11 at 19:57
  • 3
    \$\begingroup\$ @McMinton: In which case you probably want a try/finally anyway so that they will be cleaned up if an exception is thrown. \$\endgroup\$ – Jim Mischel Apr 29 '11 at 20:01
  • \$\begingroup\$ @McMinton: I use using or finally blocks for those. \$\endgroup\$ – recursive Apr 29 '11 at 20:01
4
\$\begingroup\$

Often, the specific problem dictates the solution.

  • Immediately returning can save levels of nested conditions, greatly improving legibility of the code.

  • Waiting until the end allows for simpler logging, and sometimes simpler debugging.

  • If there's a non-trivial probability you'll want to add further logic to the function later, then don't return until the end.

\$\endgroup\$
3
\$\begingroup\$

Best practice is to return as early as you can. The first example is clearer to read and you're not dragging around a variable you don't actually need.

\$\endgroup\$
2
\$\begingroup\$

Ummm... use Linq?

return items.Select(item => item.Split('|'))
            .Where(split => (split[0] == searchFor))
            .Select(split => split[1])
            .FirstOrDefault();

or

searchFor += "|";
return items.Where(item => (item.StartsWith(searchFor)))
            .Select(item => item.Substring(searchFor.Length))
            .FirstOrDefault();

BTW the second implementation is about 17% faster than the OP's explicit loop.

I realize this doesn't directly answer the question, but I thought I'd post it here for completeness. And to be cool. All the kids are replacing loops with LINQ these days.

\$\endgroup\$
1
\$\begingroup\$

Which is better is fairly subjective and depends upon context.

But...your code looks like it is the body of a procedure that looks like this...

public string Search(string searchFor, IEnumerable<string>items){
    foreach(string item in items) {
        string[] split = item.Split('|');
        if(split[0] == searchFor)
            return split[1];
    }
    return null;
}

Which is quite common, but IMO also quite wrong. By returning null when the value is not found, you are giving two semantic (and entirely opposite) meanings to the returned value: the value found, and no value was found.

public boolean TrySearch(string searchFor, IEnumerable<string>items, 
                         out string stringFound){
    stringFound=null;
    foreach(string item in items) {
        string[] split = item.Split('|');
        if(split[0] == searchFor){
            stringFound=split[0];
            return true;
        }
    }
    return false;
}

Is much safer and clearer because it lets the caller know that the return value may or may not be usable and encourages the check for null.

\$\endgroup\$
0
\$\begingroup\$

No main difference, and as far as I'm concerned no real advantage of using one or the other, besides readability, purism.

Check out this quote I found:

True purists will say that a function should only have a single entry (at the top) and exit (at the bottom) point.

Really, it gets down to whether or not you want to continue executing your code after a certain loop condition has happened. Those that use returns within a loop (and I am guilty of the occasional usage myself) simply want a quick way to exit the function without writing additional logic to support the "single exit" philosophy.

Remember, they do have different purposes as BREAK will only exit the loop that you are in but execution of the function continues.

\$\endgroup\$
0
\$\begingroup\$

I would return directly, it's more direct and more clear. You dont need to add an extra variable and have less possibilty to make a mistake.

\$\endgroup\$
0
\$\begingroup\$

If you know that that is the end of the function then by all means return. There is no nead to do a single exit point. That is old school nonsense.

\$\endgroup\$
  • \$\begingroup\$ "No need to do a single exit point" - that is ridiculous. Stick that code into a function with > 200 lines and then try to make that statement. It is by all means a best practice to have a single exit point. It makes for cleaner code and forces you to think about the structure of the program in terms of readability. \$\endgroup\$ – Adam Spicer Apr 30 '11 at 10:43
  • 1
    \$\begingroup\$ @AdamSpicer if you're concerned about readability you shouldn't be writing 200-line functions in the first place \$\endgroup\$ – Benjamin Hodgson Nov 23 '13 at 8:41
0
\$\begingroup\$

I think there's no "best practice" on this - you'll find people supporting both possibilities.

I personally would take the first approach as the control flow is better expressed using the return. You simply know, that if the condition is true, no further code will be executed after the loop.
And code which is clearer and thus easier to understand should be more maintable.

\$\endgroup\$
0
\$\begingroup\$

For this trivial example, it hardly matters, but if the function were much more complicated it would make more sense to return as quickly as possible, handling any cleanup activity in a finally block.

Opportunistic return is an important practice for avoiding the arrow anti-pattern (see technique #4 described in this article). Using opportunistic return allows you to transform nested ifs into parallel/progressive ifs and gates, which reduces cyclomatic complexity and (it's fairly well established) the likelihood of logic errors.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.