Removing XElements in a foreach loop

Question

So, I have a bug to remove

foreach (XElement x in items.Elements("x")) 
{
    XElement result = webservice.method(x);

    if (/*condition based on values in result*/) 
    {
        x.Remove();
    }
}

The problem is that calling x.Remove() alters the foreach such that if there are two Elements("x"), and the first is removed, the loop doesn't get to the second x element.

So how should I be looping this? Or should this be rewritten another way?

I actually have just modified the foreach to be "foreach (XElement x in items.Elements("x").Reverse())" and that seems to work fine as the problem before was the foreach moved the index up, and the Remove shifted everything down, causing items to be skipped. Reversing the order seems to make sense. But, I'll leave the question opened in case someone has a better solution. — CaffGeek, Oct 07 '09 at 17:00
I did a for loop where I had to do a i-- if it actually removed an item to compensate for the index. Your way with the reverse doesn't seem like a bad option either though, but I am not a .NET expert, so I am a little skeptical of what I say, lol. — Xaisoft, Oct 07 '09 at 17:09
retaged to C#3.0. There is no C# with version 3.5 (see this post for details http://stackoverflow.com/questions/247621/what-are-the-correct-version-numbers-for-c) — Vaccano, Oct 16 '09 at 19:46

score 32 · Accepted Answer · edited Oct 09 '17 at 12:27

32

I suspect that Linq may be able to help you out here as follows.

using System.Linq;

void foo()
{
    items.Elements("x")
         .Where(x => condition(webservice.method(x)))
         .Remove();
}

If that doesn't work (i.e. the internal enumerator is still invalidated), make a shallow copy of the selected elements and delete them as follows.

using System.Linq;

void foo()
{
    List xElements = items.Elements("x")
                          .Where(x => condition(webservice.method(x)))
                          .ToList();

    for (int i = xElements.Count - 1; i > -1; i--)
    {
        xElements[i].Remove();
    }
}

edited Oct 09 '17 at 12:27

Martin Schneider

14,263
7
55
58

answered Oct 07 '09 at 19:12

Steve Guidi

19,700
9
74
90

Note that each Remove() walks the internal linked list of child elements from the first child onwards, hence the computational complexity of each remove is O(N). Is there an O(1) way of removing elements? – redcalx Nov 26 '14 at 11:39
What the **** hell! I had a few foreach's to make this myself and it took roughly 15 minutes to finish everything. With your way (solution 1) it only took me 664 ms! – Jordec Apr 19 '18 at 10:49

score 1 · Answer 2 · answered Oct 07 '09 at 16:52

1

Create a collection before the loop logic, add the elements to be removed to the new collection, then call the items.Remove on each element in the new collection.

answered Oct 07 '09 at 16:52

Tom Anderson

10,807
3
46
63

This should work. I remember doing this. My answer is probably not good. I remember something about if you make changes to a list or something, it is a good idea to use for, but if you are just looping through without making any changes, foreach is fine. Is this correct? – Xaisoft Oct 07 '09 at 16:55

score 1 · Answer 3 · answered Oct 07 '09 at 16:53

1

Try it without a for instead of the foreach.

answered Oct 07 '09 at 16:53

Xaisoft

45,655
87
279
432

Serge · Answer 4 · 2023-04-25T20:33:34.670

The existing answer has a good pure-Linq alternative, but puts in too much work for the iterative approach.

You can still loop on Elements("x").ToList() in the foreach - no need to transfer it completely to a traditional for.

using System.Linq;

foreach (XElement x in items.Elements("x").ToList()) 
{
    XElement result = webservice.method(x);

    if (/*condition based on values in result*/) 
    {
        x.Remove();
    }
}

Removing XElements in a foreach loop

4 Answers4