Remove an item from a generic list while in a foreach

Question

I am coding pickups(coins, etc.) at the moment, but when you pick something up... it doesn't ly on the ground anymore.

But I am getting an exception: System.InvalidOperationException. caused by: map.Remove(), and thrown by the foreach loop.

So, how do I remove the pickup correctly from the list?

foreach (CollisionTiles tile in map.CollissionTiles)
{
    if (!tile.isTransparant)
        player.Collision(tile.Rectangle, map.Width, map.Height);
    else
    {
        if (player.PickUp(tile, map.Width, map.Height))
            map.Remove(tile);
    }

    camera.Update(player.Position, map.Width, map.Height);
}

The map.Remove() void:

public void Remove(CollisionTiles tile)
{
    this.collissionTiles.Remove(tile);
}

You can't iterate thru collection with `foreach` while modifying it. Use `for` with indexer and start from the largest index down — Yuriy Galanter, Aug 30 '13 at 16:37
BTW - Do you really want to update the camera *for each tile*, instead of once after the loop is done? — Reed Copsey, Aug 30 '13 at 16:38
@ReedCopsey I know. I did it quick, and you know what happens than... — joppiesaus, Aug 30 '13 at 16:40

score 9 · Accepted Answer · answered Aug 30 '13 at 16:37

9

The simplest approach is to remember all the elements you want to remove, then remove them later:

var tilesToRemove = new List<CollisionTiles>();
foreach (var tile in map.CollisionTiles)
{
    if (!tile.IsTransparent)
    {
        player.Collision(tile.Rectangle, map.Width, map.Height);
    }
    else if (player.PickUp(tile, map.Width, map.Height))
    {
        tilesToRemove.Add(tile);
    }
    camera.Update(player.Position, map.Width, map.Height);
}

// Remove all the ones we didn't want
foreach (var tile in tilesToRemove)
{
    map.Remove(tile);
}
// Potentially call camera.Update here? We don't know if it uses the tiles

(It's not clear why you're calling camera.Update that often, by the way - could you not call it once after the loop?)

answered Aug 30 '13 at 16:37

Jon Skeet

1,421,763
867
9,128
9,194

1

Aside from perhaps it using less memory why do you prefer Remove in a foreach to `map.Except(tilesToRemove)`? That seems like the more LINQ way of doing things... – evanmcdonnal Aug 30 '13 at 16:42
@evanmcdonnal: Well `map` isn't a list in itself, and `map.Except` doesn't actually remove items from the list. We don't know what other references there are to the same list, for example - otherwise yes, we could potentially use `map.CollisionTiles = map.CollisionTiles.Except(tilesToRemove).ToList()`. – Jon Skeet Aug 30 '13 at 16:43
Ah I didn't notice that the list was actually a property on map. I was just thinking of assigning the `IEnumerable` returned by `Except` to a local variable. – evanmcdonnal Aug 30 '13 at 16:45

score 2 · Answer 2 · answered Aug 30 '13 at 16:40

2

Use a for loop instead of a foreach:

for (int i = map.CollisionTiles.Count - 1; i >= 0; i--)
{
    CollisionTile tile = map.CollisionTiles[i];
    if (!tile.isTransparant)
        player.Collision(tile.Rectangle, map.Width, map.Height);
    else
    {
        if (player.PickUp(tile, map.Width, map.Height))
            map.Remove(tile);
    }

    camera.Update(player.Position, map.Width, map.Height);
}

answered Aug 30 '13 at 16:40

ProgramFOX

6,131
11
45
51

You're assuming it's okay to reverse the order of iteration. That may well be the case, but I think it would be better to specify that. – Jon Skeet Aug 30 '13 at 16:43
Won't `tile` not be declared? Instead of `tile` do you mean `map.CollisionTiles[i]`? – davidsbro Aug 30 '13 at 16:44

Remove an item from a generic list while in a foreach

2 Answers2