3

There's a button click event in a lengthy code I've written. I have a list of objects in it. Each time the button is clicked, the list should be modified ( for example, some items should be removed ) and then iterated using a foreach loop.

List<Person> lp=new List<Person>();
lp.RemoveAt(2);

foreach(Person j in lp)
{
    // do something
}

When I try to execute the above code, it results in an exception.

InvalidOperationException : Collection was modified; enumeration operation may not execute.

I found some solutions on the internet and tried them. One of them is,

foreach(Person j in lp.ToList())
{
    // do something
}

But nothing could stop the exception.

can someone help with this ?

Christos
  • 53,228
  • 8
  • 76
  • 108
nidarshani fernando
  • 493
  • 4
  • 10
  • 26

2 Answers2

19

Obviuosly, your do something code is trying to modify the collection.
It's generally a bad idea to modify the collection while iterating on it.

What you can do:

  1. Copy collection to another.

    foreach(Person j in lp.ToArray())

    or

    foreach(Person j in new List<Person>(lp))

  2. Use temporary collection of modified items.

    List<Person> itemsToDoSomething = new List<Person>();
foreach(Person j in lp)
    itemsToDoSomething.Add(j);

    Then apply the desired action. For instance, remove items from collection:

    lp.RemoveAll(item => itemsToDoSomething.Contains(item));
Dmitry
  • 13,797
  • 6
  • 32
  • 48
  • isn't looping over keys is a good idea ?? http://stackoverflow.com/a/26864676/1589885 – Language Lassi Mar 21 '16 at 08:09
  • new List(lp) will still copy and foreach inside. It will still throw just more unlikely! – joe Jun 10 '20 at 09:06
  • 1
    @joe Wrong. The copy of collection is a separate collection and changes of the original collection doesn't affect it. Please see the [docs](https://learn.microsoft.com/en-us/dotnet/api/system.collections.generic.list-1.-ctor?view=netcore-3.1#System_Collections_Generic_List_1__ctor_System_Collections_Generic_IEnumerable__0__): _"Initializes a new instance of the `List` class that contains elements copied from the specified collection and has sufficient capacity to accommodate the number of elements copied."_ If you think you are right, please provide an example. – Dmitry Jun 15 '20 at 21:33
  • @Dmitry Yes it's a copy. But the copy procedure itself is not thread-safe.The problem is now when you run ToArray. ToArray doesn't solve the problem, only migrates it to another problem: `Destination array was not long enough. Check destIndex and length` but just more unlikely to happen because ToArray() is way more faster without any processing inside. And the constructor version is the same implementation, by calling `CopyTo` Example here: https://dotnetfiddle.net/IqVbZ8 – joe Jun 16 '20 at 05:21
  • @Dmitry Sorry for the misunderstanding. Of course this depends on whether it's the `Dosomething` or another thread that modifies the list. And you are right if it's the first case. But the OP says ToList doesn't solve the problem. So the case is more like GMaiolo's comment. The answer should cover general cases. – joe Jun 16 '20 at 06:12
1

Try Modify the cloned list and copy the reference back (just an idea):

List<Person> newList = new List<Person>(lp);lp = newList;

Basically you are not allowed to modify a list while iterating through it

Dongming Yan
  • 123
  • 9