views:

117

answers:

5

Hey guys,

I've got a function that checks a list of objects to see if they've been clicked and fires the OnClick events accordingly. I believe the function is working correctly, however I'm having an issue:

When I hook onto one of the OnClick events and remove and insert the element into a different position in the list (typical functionality for this program), I get the "Collection was modified..." error.

I believe I understand what is going on:

  • The function cycles through each object firing OnClick events where necessary
  • An event is fired and the object changes places in the list per the hooked function
  • An exception is thrown for modifying the collection while iterating through it

My question is, how to do I allow the function to iterate through all the objects, fire the necessary events at the proper time and still give the user the option of manipulating the object's position in the list?

Thanks, Tyler

+2  A: 
  • Use copy of the collection instead of initial collection for iteration.

  • If you have a collection that supports indices (like List) you can use 'for' loop instead of iteration with 'foreach'.

Andrei Taptunov
Or, List<>.ForEach()
Alan
foreach will still cause the same error.
SnOrfus
+6  A: 

There are two general solutions to this kind of problem:

  • Make a copy of the list. Iterate over the copy.
  • Make a list of the changes that need to happen. Apply the changes after you've finished iterating.

The "use indices" option doesn't sound like it's suitable if you want to decouple the code that makes the changes from the code that does the looping.

tc.
I think the second option sounds best. I'll just record any modifications to the list during the update and then apply them after the iteration is complete.
Tyler Murry
Note that you have to be clear about exactly what a "position" means when you're doing more than one edit — "swap 1 and 2" and "swap 2 and 3", or "move C after A" and "delete A". I generally find that it's easier to make code that does the right thing by iterating over a copy, even though it may be terribly inefficient.
tc.
+2  A: 

If you are using a foreach loop to manipulate the collection, try replacing it with a

for (int a = items_count - 1; a >= 0; --a)
Yiannis Mpourkelis
this is fine only if you have a direct access to the collection item's by index.
Ron Klein
+2  A: 

The behaviour you are seeing is by design:

The foreach statement of the C# language (for each in Visual Basic) hides the complexity of the enumerators. Therefore, using foreach is recommended instead of directly manipulating the enumerator.

Enumerators can be used to read the data in the collection, but they cannot be used to modify the underlying collection.

Firing an event, which is synchronously executed has the same effect as modifying the collection from within the foreach loop.

My preference is to use a for loop going backwards, this avoids conditional logic of updating the loop index depending on whether or not you insert something:

for (var i = collection.Count - 1; i >= 0; i--) {
  if (condition)
    collection.Insert(i, item);
}

The corresponding incrementing loop will look something like this:

for (var i = 0; i < collection.Count; i++) {
  if (condition) {
    collection.Insert(i, item);
    i++;
  }
}
Igor Zevaka
+1  A: 

Collections cannot be modified when they are being iterated. You need to mark the items that are to be removed from the collection first and then remove them. You can fire the events before or after the items are removed depending upon your requirement. This code snippet shows what i am pertaining to

List itemsTobeRemoved=new List(); for (var i = 0; i < collection.Count; i++) { if (condition) { itemsTobeRemoved.Add(i); }

foreach (var i = 0 in itemsTobeRemoved) { if (condition) { collection.RemoveAt(i); }

nitroxn