tags:

views:

216

answers:

8

I want to do a foreach loop while taking out members of that foreach loop, but that's throwing errors. My only idea is to create another list inside of this loop to find which Slices to remove, and loop through the new list to remove items from Pizza.

foreach(var Slice in Pizza)
{
    if(Slice.Flavor == "Sausage")
    {
        Me.Eat(Slice); //This removes an item from the list: "Pizza"
    }
}
+4  A: 

use a for loop not a foreach

for(int i = 0; i < in Pizza.Count(), ++i)
{

    var Slice = Pizza[i];
    if(Slice.Flavor == "Sausage")
    {
        Me.Eat(Slice); //This removes an item from the list: "Pizza"
    }
}
Preet Sangha
I would iterate backwards, otherwise the "var Slice = Pizza[i];" call will throw an IndexOutOfRange or similar exception.
Michael Stum
(FYI Slight edit error with "i < in Pizza.Count()")
Kieren Johnstone
While this does solve the problem, this increases likelyhood of making off-by-one access mistakes that throw run-time exceptions. Unless you actually have a usage for the index value of an item in a list, I'd much rather see and work with "safer" code like @spender's answer.
Mike Atlas
Cheers @Michael Stum - I'll remember that.
Preet Sangha
+19  A: 

You can do this, by far the simplest way I have found (like to think I invented it, sure that's not true though ;))

foreach (var Slice in Pizza.ToArray())
{
    if (Slice.Flavor == "Sausage") // each to their own.. would have gone for BBQ
    {
        Me.Eat(Slice);
    }
}

..because it's iterating over a fixed copy of the loop. It will iterate all items, even if they are removed.

Handy isn't it!

(By the way guys, this is a handy way of iterating through a copy of a collection, with thread safety, and removing the time an object is locked: Lock, get the ToArray() copy, release the lock, then iterate)

Hope that helps!

Kieren Johnstone
GENIUSES ALL OVER THE PLACE AT SO, (and not the douchey Apple ones)Thanks so much!!! :D
Soo
oau.. this should go to the C# hall of fame tips.
Radu094
Jim Schubert
+3  A: 

Proably the clearest way to approach this would be to build a list of slices to eat, then to process it, avoiding altering the original enumeration in-loop. I've never been a fan of using indexed loops for this, as it can be error-prone.

List<Slice> slicesToEat=new List<Slice>();
foreach(var Slice in Pizza)
{
    if(Slice.Flavor == "Sausage")
    {
        slicesToEat.Add(Slice); 
    }
}
foreach(var slice in slicesToEat)
{
    Me.Eat(slice);
}
spender
+1 here for reducing the error-prone likelihood. (-1 for @Preet's answer for same reason in reverse).
Mike Atlas
+4  A: 

If you have to iterate through a list and need to remove items, iterate backwards using a for loop:

// taken from Preet Sangha's answer and modified
for(int i = Pizza.Count-1; i >= 0, i--)
{
    var Slice = Pizza[i];
    if(Slice.Flavor == "Sausage")
    {
        Me.Eat(Slice); //This removes an item from the list: "Pizza"
    }
}

The reason to iterate backwards is so that when you remove Elements you don't run into an IndexOutOfRangeException that's caused by accessing Pizza[5] on a Pizza that has only 5 Elements because we removed the sixth one.

The reason to use a for loop is because the iterator variable i has no relation to the Pizza, so you can modify the Pizza without the enumerator "breaking"

Michael Stum
+2  A: 

Perhaps change your Me.Eat() signature to take an IEnumerable<Slice>

Me.Eat(Pizza.Where(s=>s.Flavor=="Sausage").ToList());

This lets you perform the task in 1 line of code.

Then your Eat() could be like:

public void Eat(IEnumerable<Slice> remove)
{
    foreach (Slice r in remove)
    {
        Pizza.Remove(r);
    }
}
p.campbell
How does this solve the problem? **The sequence is still being modified while it is being iterated**. Remember, Pizza.Where is lazy; it does not loop over the collection until asked, and no one asks to loop over the collection until the foreach in Eat, which then does the removal. What you want to do is calculate the set of things to be removed *eagerly* and then remove them *after* the set has been calculated, not *while* it is being calculated.
Eric Lippert
@Eric: thank you for paying attention here on this. I've made a change to the answer whereby the calculation of slices to be removed is actually created eagerly, not lazy. Any thoughts or comments is appreciated!
p.campbell
A: 

The VB6-styled "Collection" object allows for modification during enumeration, and seems to work sensibly when such modifications occur. Too bad it has other limitations (key type is limited to case-insensitive strings) and doesn't support generics, since none of the other collection types allow modification.

Frankly, I'm not clear why Microsoft's iEnumerable contract requires that an exception be thrown if a collection is modified. I would understand a requirement that an exception be thrown if a changes to a collection would make it impossible for an enumeration to continue without wackiness (skipping or duplicating values that did not change during enumeration, crashing, etc.) but see no reason not to allow a collection that could enumerate sensibly to do so.

supercat
A: 

Where can you order pizza where slices have separate toppings? Anyway...

Using Linq:

// Was "Me.Eat()" supposed to be "this.Eat()"?
Pizza
    .Where(slice => slice.Flavor == "Sausage")
    .Foreach(sausageSlice => { Me.Eat(sausageSlice); });    

The first two lines create a new list with only sausage slices. The third will take that new subset and pass each slice to Me.Eat(). The { and ;} may be superfluous. This is not the most efficient method because it first makes a copy (as do many other approaches that have been given), but it is certainly clean and readable.

BTW, This is only for posterity as the best answer was already given - iterate backward by index.

Phil Gilmore
A: 

What kind of collection is Pizza? If it's a List<T> then you can call the RemoveAll method:

Pizza.RemoveAll(slice => string.Equals(slice.Flavor, "Sausage"));
Sam