Looping through a collection

Question

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;

Answer 1

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.

Answer 2

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.

Answer 3

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.

Answer 4

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.

Answer 5

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.

Answer 6

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.

Answer 7

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.

Answer 8

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.

Answer 9

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.

Answer 10

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.