3

Possible Duplicate:
What is the best way to modify a list in a foreach?

Suppose I have an ObservableCollection mycollection and I want to do something by enumerator like:

foreach(var x in mycollection)
{
   // This will do something based on data of x and remove x from mycollection
   x.Close();
}

The method Close() has one line of code - mycollection.Remove(x);. When I run this code, get the following error:

Collection was modified; enumeration operation may not execute.

I can't change method Close() because it is called in a lot of other places in the application. How can I resolve this issue?

Community
  • 1
  • 1
KentZhou
  • 24,805
  • 41
  • 134
  • 200

3 Answers3

8

You can't remove items while enumerating the collection. One simple solution is to use a for loop going from the last item to the first and remove as necessary:

for (int i = myCollection.Count - 1; i >= 0; i--)
{
    var item = myCollection[i];
    if (ShouldDelete(item))
    {
        item.Close();
    }
}
Adi Lester
  • 24,731
  • 12
  • 95
  • 110
2

Two ways you could approach this:

  1. If you don't need to remove everything from the list, use your foreach to populate a separate list of "items to be closed," and call .Close for each item in the second list when you're done. That way, you avoid modifying mycollection while you enumerate.

  2. If you're calling Close on every item in the list, try this instead:

    while (mycollection.Count > 0
{ 
     mycollection[0].Close()) 
}
Dan Puzey
  • 33,626
  • 4
  • 73
  • 96
  • 1
    As an aside, I'd be very tempted to change the Close() method to not be responsible for removing the item from the collection. This code has the potential to be very confusing, since it makes this loop like an infinite loop - it's not immediately apparent that the collection is being modified (you have to know the internal details of Close), which makes the loop condition look like it's either always true or always false. – Chris Aug 01 '12 at 14:01
  • @Chris: agreed, though there are certain cases where this might make sense (say, a connection pool where closing the connection removes it from the pool). There are definitely more elegant ways of structuring this code, though. – Dan Puzey Aug 01 '12 at 14:04
0

In the foreach, use mycollection.ToList() instead.

Eren Ersönmez
  • 38,383
  • 7
  • 71
  • 92
  • 1
    I'm not familiar with the ToList() method, but it either does one of two things: firstly, it could just cast the collection to a list and return it, in which case you'll get the same exception as with the original code. Alternatively, it could be copying the collection into a brand new list, in which case you're copying the entire collection on every iteration of the loop which is incredibly inefficient. Either way, this is not a good answer. – Chris Aug 01 '12 at 13:58
  • 1
    @Chris that's not correct. this copies the list **once**, not on every iteration. It is a perfectly valid and simple way to get around the invalidated enumerator problem, and is used quite commonly. If special optimization is needed, surely using a for loop would be more efficient. However, just the fact that an `ObservableCollection` is being used tells me that is likely not the case here. – Eren Ersönmez Aug 01 '12 at 19:07