Remove element from List within foreach

Question

I have a List<Buoy> and I want to remove the Buoy that is hit by my Ship.

foreach (var Chest in Chests) {
    if (Chest.Bounds.Intersects (Ship.Bounds)) {
        Player.Score += 1;
    }
}

I have written the Dispose()-method and I want to set the specific Chest to null. How can I do this, without getting the error I am assigning something to Chest because it is a foreach iteration variable.

CodeCaster: I have written this question, because I didn't understand the above question, as it is much more extensive as the work I am doing. — user3710669, Jun 05 '14 at 18:06
What part don't you understand? `for (int i = Chests.Count - 1; i >= 0; i--)` will work. — CodeCaster, Jun 05 '14 at 18:08
CodeCaster: I don't understand it, because if I have three `Ship`s on my screen and I hit the second in the list, how can I remove it based on which one I hit? — user3710669, Jun 05 '14 at 18:09
Nevermind, I misread (not in the least because your title (remove) doesn't match your question (set null)). There most definitely is another duplicate somewhere. Are you sure you want to set `Chest` to `null` though? — CodeCaster, Jun 05 '14 at 18:11
@CodeCaster: Think of it as picking up a PowerUp. You hit it with the Ship and it disappears, thus being removed from memory. I thought the way to go was using null? — user3710669, Jun 05 '14 at 18:13
And then in the next iteration of `foreach (var Chest in Chests)` you check each `Chest` for `null`, right? Otherwise `Chest.Bounds` will throw a NullReferenceException. Easier to remove them from the list if you won't need them again... — CodeCaster, Jun 05 '14 at 18:14
No, I'm checking whether any of the Chests is in collision with Ship in the `foreach`, and if there is Collision, I want to remove the Chest that is being hit. — user3710669, Jun 05 '14 at 18:17

LVBen · Answer 1 · 2014-06-05T18:25:12.870

4

foreach is great, but it does have an unfortunate side effect of making the iterator immutable. One workaround would be to use another list to keep track of which Buoys to remove.

     List<Buoy> ChestsToRemove = new List<Buoy>();
     foreach (Buoy chest in Chests)
     {
        if (Buoy.Bounds.Intersects(Ship.Bounds))
        {
           Player.Score += 1;
           ChestsToRemove.Add(chest);
        }
     }
     foreach (Buoy chestToRemove in ChestsToRemove)
     {
        Chests.Remove(chestToRemove);
     }

edited Jun 05 '14 at 18:25

answered Jun 05 '14 at 18:19

LVBen

2,041
13
27

1

The "Kill list" approach is also a very common way of solving this problem. +1 – BradleyDotNET Jun 05 '14 at 18:20

score 4 · Answer 2 · answered Jun 05 '14 at 18:20

4

There isn't a real need to couple the discovery of which things you're intersecting with removal from the list of all things.

var collisions = Chests.Where(c => c.Bounds.Intersects(Ship.Bounds)).ToList();
Player.Score += collisions.Count();
Chests = Chests.Except(collisions);

answered Jun 05 '14 at 18:20

Preston Guillot

6,493
3
30
40

2

If `Chests` is a `List` and not an `IEnumerable` you'll need to call `ToList()` on the last line as well. – Preston Guillot Jun 05 '14 at 18:25

score 1 · Answer 3 · answered Jun 06 '14 at 06:24

1

you can also remove like on example below

foreach (var Chest in Chests) {
    if (Chest.Bounds.Intersects (Ship.Bounds)) {
        Player.Score += 1;
        Chest.IsActive = false;
    }
}

Chests.RemoveAll(c => !c.IsActive)

answered Jun 06 '14 at 06:24

Davor Mlinaric

1,989
1
19
26

Complexity · Answer 4 · 2014-06-05T18:13:24.100

0

The problem when you want to remove items from a list you're iterating over is that the enumerator loses track of where you are in the list.

It's a bit like sawing a branch on which you are sitting. You shoudn't do it because it will have unecptected behaviour.

Now, how can this be solved?

Quite easily, you need make a copy of your list, iterate over your copy, and remove the items from your original list.

Sounds like a lot of code, but the only thing that you need to do is add ToList() in your foreach statement.

Like so:

foreach (var Chest in Chests.ToList()) {
    if (Chest.Bounds.Intersects (Ship.Bounds)) {
        Player.Score += 1;
    }
}

Now, why does this work?

It's because ToList() creates a copy of your list in memory. So basiclly, your iterating over a copy of the collection right now.

So this apporach should work.

edited Jun 05 '14 at 18:13

answered Jun 05 '14 at 18:01

Complexity

5,682
6
41
84

Chests doesn't have the method `ToList()`, as it already is a List. ;) – user3710669 Jun 05 '14 at 18:02
1

But it doesn't answer the question besides adding the needed "ToList()" with no explanation as to *why* that is important. Its also a very poor common answer. The OP's comment to that effect makes it painfully clear that the why is important, and missing. – BradleyDotNET Jun 05 '14 at 18:08
I'm editing my answer, I just was in a hurry. I'm sorry, I just wanted to help the person :-) – Complexity Jun 05 '14 at 18:10
Much, much better now. Thank you for the edit! You could add the remove code as well, since setting to null is not generally the right approach. – BradleyDotNET Jun 05 '14 at 18:18
You're welcome. As as I told, I'm sorry for the poor answer :-) – Complexity Jun 05 '14 at 18:19
Chests should be an IList normally instead of a List. Is there a possibility for you to change? – Complexity Jun 05 '14 at 18:21
I can change that to an IList, I suppose. ;) – user3710669 Jun 05 '14 at 18:22
Than it's a good idea to do so, since an IList is an interface, so your list can be easily swapped with some other custom implementation of IList – Complexity Jun 05 '14 at 18:22

Remove element from List within foreach

4 Answers4