0

Here's an easy one for one of you smart folks -- I have an observable collection containing viewmodel objects. I'm trying to go through the objects and remove any where the plant.Living property is "No". I am using this code:

        foreach (PlantViewModel plant in Plants)
        {
            if (plant.Living == "No") 
            {
            Plants.Remove(plant);  
            }
        }
        PlantsViewSource.Source = Plants;
        PlantsGridView.SelectedItem = null;

However, when the first object is encountered that meets the criteria and that object is removed, it modifies the collection and the foreach throws an error. How can I remove the objects from the collection in another way?

wheezer
  • 135
  • 1
  • 2
  • 9
  • use a `for` loop instead – Flat Eric Jul 30 '14 at 17:00
  • 1
    At current moment it post shows lack of effort to find an answer (which is unfortunate as you've probably spend a lot of time trying to find solution and solve it). It is very surprising that there are no links to similar questions in the post. Please try search like http://www.bing.com/search?q=c%23+foreach+collection+modified. – Alexei Levenkov Jul 30 '14 at 17:03

1 Answers1

2

As the error tells you, you can't remove an item from a collection you're enumerating. An answer is to keep a list of the items you want to remove. 

List<PlantViewModel> plantsToRemove = new List<PlantViewModel>();

foreach (PlantViewModel plant in Plants)
        {
            if (plant.Living == "No") 
            {
            plantsToRemove.Add(plant);  
            }
        }

         foreach(var plant in plantsToRemove)
             Plants.Remove(plant);

        PlantsViewSource.Source = Plants;
        PlantsGridView.SelectedItem = null;

A more readable option is;

List<PlantViewModel> plantsToRemove = Plants.Where(p => p.Living == "No");

foreach(var plant in plantsToRemove)
                 Plants.Remove(plant);

            PlantsViewSource.Source = Plants;
            PlantsGridView.SelectedItem = null;
tbddeveloper
  • 2,407
  • 1
  • 23
  • 39
  • 1
    You might want to add an explanation, such as "Temporarily store the items to remove in a separate list, then remove them by looping over the list that contains the items to remove. This avoids modifying the collection as you're looping over it." – mason Jul 30 '14 at 17:02
  • Thank you Mr. Hammerstein. Quick too! Sorry for being ignorant. – wheezer Jul 30 '14 at 17:02
  • No need to apologize. Your question got marked because it's a common question to ask. No worries. – tbddeveloper Jul 30 '14 at 17:03
  • 1
    I was making a separate answer. There's no need to make a list of ones to remove. Just have your other list be ones to add, and only put them in that list if `plant.Living.ToUpper() != "NO"`. Then you don't have to iterate again. A cleaner way still would be with LINQ: `PlantsViewSource.Source = Plants.Where(p => p.Living.ToUpper() != "NO");` – krillgar Jul 30 '14 at 17:07