Removing an object from a list in a foreach loop of that list

Question

I have made a method that eliminates any replicates of the same string in a List.

now, the problem is that it gives me this error:

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

I read in the internet, and i think that the problem is the i am removing an object from the list inside the foreach loop of the list.

    foreach (string r in list)
    {
        int numberOfAppearance=0;
        foreach (string rs in list)
        {
            if (r == rs && numberOfAppearance> 0)
                list.Remove(rs);
            else
                numberOfAppearance++;
        }
    }

How can i fix the method? Thanks for the help

Do you need to do this yourself, or could you just use list = list.Distinct().ToList()? Or are you trying to remove all occurrences of any items that have duplicates? (So should a list of { "a", "b", "c", "b" } end up as { "a", "b", "c" } or just { "a", "c" }?) — Jon Skeet
– Jon Skeet, Commented Mar 29, 2014 at 10:38
ohhh there is a method that does that?!, i guess that would work, i dont need to do this myself. Thanks — Cooper
– Cooper, Commented Mar 29, 2014 at 10:40
Well it depends on what you want the result to be. Using Distinct will give {"a", "b", "c"} in this case. — Jon Skeet
– Jon Skeet, Commented Mar 29, 2014 at 10:40

Jon Skeet · Accepted Answer · 2014-03-29 11:08:09Z

Firstly, as noted in comments, LINQ has got you covered here:

list = list.Distinct().ToList();

It's well worth looking into LINQ for data operations - it can make things much simpler.

As for what's wrong with your current code - there are a couple of things:

Firstly, you're removing by item rather than by index, which will remove the first occurrence of that item, not the one you're actually looking at

Secondly, if you modify a list while you're iterating over it, you will get precisely the exception you've seen. From the docs for List<T>.GetEnumerator:

An enumerator remains valid as long as the collection remains unchanged. If changes are made to the collection, such as adding, modifying, or deleting elements, the enumerator is irrecoverably invalidated and its behavior is undefined.

You can get around this by iterating by index rather than using a foreach loop, but if you're removing an item you need to remember that everything below that will move up one element. So either you need to iterate backwards to remove items, or you need to remember to decrement the index.

Here's an approach which uses iterating by index forwards in terms of what we're looking at, but backwards in terms of looking for duplicates - stopping when we get to the index we're looking at. Note that this is still O(N²) - it's not as efficient as using Distinct:

// We're looking for duplicates *after* list[i], so we don't need to go as far
// as i being the very last element: there aren't any elements after it to be
// duplicates. (We could easily still just use list.Count, and the loop for j
// would just have 0 iterations.)
for (int i = 0; i < list.Count - 1; i++)
{
    // Go backwards from the end, looking for duplicates of list[i]
    for (int j = list.Count - 1; j > i; j--)
    {
        if (list[j] == list[i])
        {
            list.RemoveAt(j);
        }
    }
}

(For more details on Distinct, see my Edulinq post on it.)

@Default: We're looking for elements after list[i] - so when i is at the end of the list, there can't be any later duplicates. In your example, the second "a" would have been removed when i was 0.

Automatico · Accepted Answer · 2014-03-29 10:54:44Z

As many people point out, you can use the Distinct method for your particular problem.

However, the problem you are actually having is that you are trying to modify the list when you iterate over it, which will not end well.

//This will not work.
foreach (string rs in list)
{
    if (some_test)
    {
        list.Remove(rs); //Because of this line.
    }
}

If you want do do something similar to this you need to find a way around this problem. Often it involves making a new array.

For this examle you can do the following

List newList = new ArrayList();
foreach (string rs in list)
{
    if (!some_test)
    {
        newList.add(rs);
    }
}

If you really want to create a "remove duplicates" method I would have done it in this fashion (pseudocode):

Hash cache_hash = new Hash(default false)
List new_list = new List
foreach string s in list
{
    if not cache_hash[s]
    {
        new_list.add(s)
        cache_hash[s] = true
    }
}

list = new_list

This method is Ω(N) , so it is fairly fast on even large lists.

Collectives™ on Stack Overflow

Removing an object from a list in a foreach loop of that list

2 Answers 2

1 Comment

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

1 Comment

Comments

Your Answer

Sign up or log in

Post as a guest

Related