Collection mutation in foreach upon exiting loop: is it an acceptable pattern?

Question

It's well known that the mutation of a collection within an iteration loop is not allowed. The runtime will throw an exception when, for instance, an item is removed.

However, today I was surprised to notice that there's no exception if the mutating operation is followed by any exit-loop statement. That is, the loop ends.

//this won't throw!
var coll = new List<int>(new[] { 1, 2, 3 });
foreach (var item in coll)
{
    coll.RemoveAt(1);
    break;
}

I watched at the framework code, and it's pretty clear that the exception is thrown only when the iterator will moved forward.

My question is: the above "pattern" could be considered an acceptable practice, or is there any sneaky problem on using it?

Answer 1

In the example you gave, and to answer the question "Is it good practice/acceptable to modify a collection during enumeration if you break after the change", it is fine to do this, as long as you are aware of the side-effects.

The primary side effect, and why I would not recommend doing this in the general case, is that the rest of your foreach loop doesn't execute. I would consider that a problem in almost every instance of a foreach that I have used.

In most (if not all) instances where you could get away with this, a simple if check would suffice (as Servy has in his answer), so you may want to look at what other options you have available if you find yourself writing this kind of code a lot.

The most common general solution is to add to a "kill" list, and then remove after your iteration:

List<int> killList = new List<int>();
foreach (int i in coll)
{
   if (i < 0)
      killList.Add(i);

    ...
}

foreach (int i in killList)
    coll.Remove(i);

There are various ways to make this code shorter, but this is the most explicit way of doing it.

You can also iterate backwards, which won't cause the exception to be thrown. This is a neat workaround, but you may want to add a comment explaining why you are iterating backwards.

Answer 2

So your example can be relied on to work, for starters. Mutating a collection while iterating fails when you go to ask for the next item. Since this provably never asks for another item after it mutates the list, we know that won't happen. Of course, the fact that it works doesn't mean that it's clear, or that it's a good idea to use it.

What this is trying to do is remove the second item if there is an item to remove. It is designed to not break when trying to remove an item from a collection without two items. This is not a well designed way of doing that though; it's confusing to the readers and doesn't effectively convey its intentions. A much clearer method of accomplishing the same goal is something like the following:

if(coll.Count > 1)
    coll.RemoveAt(1);

In the more general case, such a Remove in a foreach can only ever be used to remove one item, so for those cases you're better off transforming the forech into an if that validates that there is an item to remove (if needed, as it is here), and then a call to remove that single item (which may involve a query to find the item to remove, instead of using a hard coded index).