CODEWITHSUNDEEP
c#linqforeach
scapegoat17
scapegoat17

Reputation: 5821

Removing from a collection while in a foreach with linq

From what I understand, this seems to not be a safe practice...

I have a foreach loop on my list object that I am stepping through. Inside that foreach loop I am looking up records by an Id. Once I have that new list of records returned by that Id I do some parsing and add them to a new list.

What I would like to do is not step through the same Id more than once. So my thought process would be to remove it from the original list. However, this causes an error... and I understand why.

My question is... Is there a safe way to go about this? or should I restructure my thought process a bit? I was wondering if anyone had any experience or thoughts on how to solve this issue?

Here is a little pseudocode:

_myList.ForEach(x => 
{
    List<MyModel> newMyList = _myList.FindAll(y => y.SomeId == x.SomeId).ToList();

    //Here is where I would do some work with newMyList

    //Now I am done... time to remove all records with x.SomeId
    _myList.RemoveAll(y => y.SomeId == x.SomeId);
});

I know that _myList.RemoveAll(y => y.SomeId == x.SomeId); is wrong, but in theory that would kinda be what I would be looking for.

I have also toyed around with the idea of pushing the used SomeId to an idList and then have it check each time, but that seems cumbersome and was wondering if there was a nicer way to handle what I am looking to do.

Sorry if i didnt explain this that well. If there are any questions, please feel free to comment and I will answer/make edits where needed.

Upvotes: 2

Views: 742

Answers (2)

BgrWorker
BgrWorker

Reputation: 679

First of all, a list inside a foreach is immutable, you can't add or delete content, nor rewrite an element. There are a few ways you could handle this situation:

GroupBy

This is the method I would use. You can group your list by the property you want, and iterate through the IGrouping formed this way

var groups = list.GroupBy(x => x.yourProperty);
foreach(var group in groups)
{
//your code
}

Distinct properties list

You could also save properties in another list, and cycle through that list instead of the original one

var propsList = list.Select(x=>x.yourProperty).Distinct();
foreach(var prop in propsList)
{
    var tmpList = list.Where(x=>x.yourProperty == prop);
    //your code
}

While loop

This will actually do what you originally wanted, but performances may not be optimal

while(list.Any())
{
    var prop = list.First().yourProperty;
    var tmpList = list.Where(x=>x.yourProperty == prop);
    //your code
    list.RemoveAll(x=>x.yourProperty == prop);
}

Upvotes: 1

Ocelot20
Ocelot20

Reputation: 10800

First off, using ForEach in your example isn't a great idea for these reasons.

You're right to think there are performance downsides to iterating through the full list for each remaining SomeId, but even making the list smaller every time would still require another full iteration of that subset (if it even worked).

As was pointed out in the comments, GroupBy on SomeId organizes the elements into groupings for you, and allows you to efficiently step through each subset for a given SomeId, like so:

_myList.GroupBy(x => x.SomeId)
       .Select(g => DoSomethingWithGroupedElements(g));

Jon Skeet has an excellent set of articles about how the Linq extensions could be implemented. I highly recommend checking it out for a better understanding of why this would be more efficient.

Upvotes: 3

Related Questions