17

Similar to my question about returning from inside a using statement (whose answer was generally "yes, it's ok") I'm wondering if returning from inside a foreach statement is similarly devoid of side-effects and considered accepted practice, or when I do this am I leaving a pointer hanging in the middle an enumeration somewhere internally, etc.

Here's an example:

public string GetCurrentTransaction(string idText)
{
    foreach (var transaction in transactions)
    {
        if (idText.IsEquivalentTo(transaction.IdText))
        {
            return transaction.Content;
        }
    }
    return "";
}
Community
  • 1
  • 1
Edward Tanguay
  • 189,012
  • 314
  • 712
  • 1,047

5 Answers5

19

Nope, dont see any issue with that.

From foreach, in (C# Reference)

A foreach loop can also be exited by the goto, return, or throwstatements.

Adriaan Stander
  • 162,879
  • 31
  • 289
  • 284
5

As long as nothing there implements IDisposable (or you have a using block around it), then that should be fine.

As far as I know, it's a fairly common and accepted practice and, as Astander mentions in his post, the documentation for foreach condones it as a legitimate practice.

Justin Niessner
  • 242,243
  • 40
  • 408
  • 536
  • 2
    Actually, `foreach` will dispose the `IEnumerator<>`, so that you're fine if the enumerator itself is disposable. See also the accepted answer in this question: http://stackoverflow.com/questions/232558/why-ienumerator-of-t-inherts-from-idisposable-but-non-generic-ienumerator-does-n – Lucero Mar 04 '10 at 12:59
2

other than it being a small code smell to return from multiple points in methods (adds to the methods cyclomatic complexity) there is no technical reason to worry about.

AndreasKnudsen
  • 3,453
  • 5
  • 28
  • 33
  • +1 I agree, it will add some complexity. I would create a string variable and assign it when the text is found and break from the loop. But from a technical standpoint, nothing to worry about (except for IDisposable as others have mentioned). Its probably more a coding style question. – SwDevMan81 Mar 04 '10 at 12:53
  • 4
    for simple "find the element and return it or return null if not found" i think it's ok to have 2 exit points as those functions are short enough so you can spot both return points easily and you don't have to carry another "returnValue" variable around. –  Mar 04 '10 at 12:54
  • 2
    IMHO, there is no code smell here. It's more readable then writing return values to variables and continuing to the end when you want to make sure the nothing more is done. And it's more readable then having tons of nested if statements (if (stillNotFound) stuff). – Stefan Steinegger Mar 04 '10 at 13:14
1

I don't know, but I'll make an educated guess: Since an enumerator typically doesn't implement IDisposable, it should be simply garbage-collected, because otherwise each use of that enumerator would leak unmanaged resources. Of course, technically, you can implement an enumerator that has side-effects on its own...

In other words, I never felt bad about returning from within a foreach block. I'd expect the language to handle things, just like with a using statement, where the language ensures that the object is disposed of (by implicitly calling Dispose in a finally block).

OregonGhost
  • 23,359
  • 7
  • 71
  • 108
  • Enumerators (inheriting from `IEnumerable<>`) do implement `IDisposable`. – Lucero Mar 04 '10 at 13:02
  • `IEnumerator` is derived from `IDisposable`, true (didn't know that), but `IEnumerator` is not. However, `foreach` starting with C# 2.0 calls Dispose in the `finally` block, so the language handles it, as I'd expect. You can safely return from a `foreach`block. – OregonGhost Mar 04 '10 at 15:19
0

As far as i remember the enumeration stays on this position until the next foreach loop. This is however no problem as any following foreach returns the position back to the start of the enumeration. In short: It has no bad side effects unless you rely on IEnumerator.Current to have a specific value (which would be bad anyways).