47

I have code that I want to look like this:

List<Type> Os;

...

foreach (Type o in Os)
    if (o.cond)
        return;  // Quitting early is important for my case!
    else
        Os.Remove(o);

... // Other code

This doesn't work, because you cannot remove from the list when you are inside a foreach loop over that list:

Is there a common way to solve the problem?

I can switch to a different type if needed.

Option 2:

List<Type> Os;

...

while (Os.Count != 0)
     if (Os[0].cond)
         return;
     else
         Os.RemoveAt(0);

... // Other code

Ugly, but it should work.

Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
BCS
  • 75,627
  • 68
  • 187
  • 294

17 Answers17

60

You can iterate through the list backwards:

for (int i = myList.Count - 1; i >= 0; i--)
{
    if (whatever) myList.RemoveAt(i);
}

In response to your comment about wanting to quit when you find an item that you're NOT removing, then just using a while loop would be the best solution.

Jon B
  • 51,025
  • 31
  • 133
  • 161
  • I think the first and second example show that the writer doesn't care about the order of the items. – Ethan Heilman May 06 '09 at 22:58
  • 5
    He iterates backward to keep a correct index "i" into the list. If you iterate forward, you need to backtrack once (i--) for every removal to avoid skipping items. – Lucas May 06 '09 at 23:11
  • doesn't work, I want to quit as soon as I find something that isn't to be removed. Aside from that minor point +1 – BCS May 06 '09 at 23:34
  • This is a good way to remove elements in a list, but in this case it doesn't work because BCS needs to remove from the start, until a condition is met. – Meta-Knight May 06 '09 at 23:36
  • Yeah, I missed that in his question. The while loop is the way to go. – Jon B May 06 '09 at 23:44
55

You should never remove anything from a collection you are iterating over while inside of a foreach loop. It's basically like sawing the branch you are sitting on.

Use your while alternative. It is the way to go.

BCS
  • 75,627
  • 68
  • 187
  • 294
User
  • 30,403
  • 22
  • 79
  • 107
  • 1
    Yup, and .NET's kinda brutal about it when you try and alter the List (it throws) – BCS May 06 '09 at 22:51
31

Do you really need to do this within a foreach loop?

This will achieve the same results as your examples, ie, remove all items from the list up until the first item that matches the condition (or remove all items if none of them match the condition).

int index = Os.FindIndex(x => x.cond);

if (index > 0)
    Os.RemoveRange(0, index);
else if (index == -1)
    Os.Clear();
LukeH
  • 263,068
  • 57
  • 365
  • 409
  • I like that solution, I'll have to think about how it interacts with my details. – BCS May 06 '09 at 23:41
15

I am a Java programmer, but something like this works:

List<Type> Os;
List<Type> Temp;
...
foreach (Type o in Os)
    if (o.cond)
        Temp.add(o);
Os.removeAll(Temp);  
Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
Milhous
  • 14,473
  • 16
  • 63
  • 82
  • I like this - it's cleaner than the backwards iteration solution. – Cristian Diaconescu Mar 03 '10 at 14:57
  • My first thought was just like that. – Krzysztof Jabłoński Jan 13 '13 at 21:29
  • Great! Was looking for just this type of thing. – Dave Baghdanov Mar 27 '13 at 21:08
  • 3
    Unless you work with tiny collections, **this is inefficient**. Whereas Jon B's solution is O(n), this one would be [O(2n + m)](http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/util/AbstractCollection.java#AbstractCollection.removeAll%28java.util.Collection%29), where *m* is the number of matching items. I say *would be*, because it does not even work in .net. There is no `List.RemoveAll(ICollection)` like there is in Java. **Take a look at @dirkgently's solution**.Unfortunately `RemoveAll` does not return the removed elements, say as an `IEnumerable`. – Evgeniy Berezovsky Nov 28 '13 at 05:39
  • @CristiDiaconescu Apart from being more efficient than this solution (which looks rather clumsy and unclean to me, with temporary data structures, thus taking up additional memory btw), why would *going forward once* be any cleaner than *going backwards once*? – Evgeniy Berezovsky Nov 28 '13 at 05:44
  • @Milhous Apart from the fact that your solution does not work in .net for the reason I mentioned above, in Java you would simply use `Iterator.remove()`, yielding a clean and efficient (cpu&mem) solution. – Evgeniy Berezovsky Nov 28 '13 at 06:01
  • @EugeneBeresovksy I don't know, that's how it looked to me *3.5 years ago*. Today, I'd fancy the immutable solution and just use `var newList = Os.SkipWhile(o=>o.Cond);`. It really depends on how this is used in the bigger picture. – Cristian Diaconescu Nov 28 '13 at 08:25
  • @EugeneBeresovksy ...and apparently *leppie* (way) below thinks exactly the same :) – Cristian Diaconescu Nov 28 '13 at 08:27
13

I just had that problem with my analysis library. I tried this:

for (int i = 0; i < list.Count; i++)
{                
   if (/*condition*/)
   {
       list.RemoveAt(i);
       i--;
   }
}

It's pretty simple but I haven't thought of any breaking point.

Anzurio
  • 16,780
  • 3
  • 39
  • 49
12

I know you asked for something else, but if you want to conditionally remove a bunch of elements you can use lambda expression:

Os.RemoveAll(o => !o.cond);
Day_Dreamer
  • 3,311
  • 7
  • 34
  • 61
  • This is a nice, clean, concise approach introduced in .NET 4.0, and is my preference. Your collection must be of type List. Here's a MSDN link on List.RemoveAll() -- https://msdn.microsoft.com/en-us/library/wdka673a(v=vs.110).aspx – iCode Nov 04 '15 at 15:03
12

Here is the EASIEST SOLUTION with the simpliest WHY

PROBLEM:

Typically, we are removing from the original list, this produces the problem of maintaining the list count and iterator location.

List<Type> Os = ....;
Os.ForEach(
    delegate(Type o) {
        if(!o.cond) Os.Remove(o);
    }
);

SOLUTION - LINQ.ForEach:

Note all I've added was ToList(). This creates a new list that you perform ForEach on, therefore you can remove your original list, yet keep iterating through the entire list.

List<Type> Os = ....;
Os.ToList().ForEach(
    delegate(Type o) {
        if(!o.cond) Os.Remove(o);
    }
);

SOLUTION - Regular foreach:

This technique also works for regular foreach statements.

List<Type> Os = ....;
foreach(Type o in Os.ToList()) {
  if(!o.cond) Os.Remove(o);
}

Please note, that this solution won't work if your original List contains struct element.

KyleMit
  • 30,350
  • 66
  • 462
  • 664
sonjz
  • 4,870
  • 3
  • 42
  • 60
9
 Os.RemoveAll(delegate(int x) { return /// });
dirkgently
  • 108,024
  • 16
  • 131
  • 187
  • How does that help? I want o remove only up to the first failed instance. – BCS May 06 '09 at 22:53
  • 1
    Why'd you want a foreach then? Use List.Remove() if all you are bothered about is the first item. Also, the return statements in your sample code look ominous: you'd return without removing the first incorrect item, if it is not the first item in the list. Is that what you want? – dirkgently May 06 '09 at 22:58
  • Also, can you tell me how this is any different from the top answer? – dirkgently May 06 '09 at 23:00
  • It's not. I don't think Jon B answers the question either ;-) – Meta-Knight May 06 '09 at 23:32
  • What I want is to remove items from the list that fit come condition and quit as soon as I find an item that doesn't. – BCS May 06 '09 at 23:36
  • @BCS: Look again at your sample code, and your comments here -- they are all different when you read thoroughly. The problem specification is poor to begin with, make up your mind on what you want. – dirkgently May 07 '09 at 04:23
4

I'd try finding the index of first item that does not satisfy the predicate and do RemoveRange(0, index) on it. If nothing else, there should be less Remove calls.

bh213
  • 6,343
  • 9
  • 43
  • 52
3

Update: Added for completeness

As several have answered, you shouldn't modify a collection while iterating it with GetEnumerator() (example foreach). The framework prevent you from doing this by throwing an exception. The generic colution to this is to iterate "manually" with for (see other answers). Be careful with your index so you don't skip items or re-evaluate the same one twice (by using i-- or iterating backward).

However, for your specific case, we can optimize the remove operation(s)... original answer below.


If what you want is to remove all items until one meets a given condition (that's what your code does), you can do this:

bool exitCondition;

while(list.Count > 0 && !(exitCondition = list[0].Condition))
   list.RemoveAt(0);

Or if you want to use a single remove operation:

SomeType exitCondition;
int index = list.FindIndex(i => i.Condition);

if(index < 0)
    list.Clear();
else
{
    exitCondition = list[0].State;
    list.RemoveRange(0, count);
}

Note: since I'm assuming that item.Condition is bool, I'm using item.State to save the exit condition.

Update: added bounds checking and saving exit condition to both examples

BCS
  • 75,627
  • 68
  • 187
  • 294
Lucas
  • 17,277
  • 5
  • 45
  • 40
  • The while loop will run out of bounds but that's easy to fix. Also I'd need a check after it to know what the termination condition was. – BCS May 06 '09 at 23:44
  • @Lucas, Your second example should also handle the case where no items match the condition, ie, where FindIndex returns -1. – LukeH May 06 '09 at 23:46
  • 1
    it was just sample code, but i will add bounds checking, thanks :) – Lucas May 06 '09 at 23:49
2

you can do it with linq

MyList = MyList.Where(x=>(someCondition(x)==true)).ToList()
mLar
  • 2,967
  • 2
  • 22
  • 23
1

Anzurio's solution is probably the most straightforward, but here's another clean one, if you don't mind adding a bunch of interfaces/classes to your utilities library.

You can write it like this

List<Type> Os;
...
var en = Os.GetRemovableEnumerator();
while (en.MoveNext())
{
    if (en.Current.Cond)
        en.Remove();
}

Put the following infrastructure, inspired by Java's Iterator<T>.remove, into your utility library:

static class Extensions
{
    public static IRemovableEnumerator<T> GetRemovableEnumerator<T>(this IList<T> l)
    {
        return new ListRemovableEnumerator<T>(l);
    }
}

interface IRemovableEnumerator<T> : IEnumerator<T>
{
    void Remove();
}

class ListRemovableEnumerator<T> : IRemovableEnumerator<T>
{
    private readonly IList<T> _list;
    private int _count;
    private int _index;
    public ListRemovableEnumerator(IList<T> list)
    {
        _list = list;
        _count = list.Count;
        _index = -1;
    }

    private void ThrowOnModification()
    {
        if (_list.Count != _count)
            throw new InvalidOperationException("List was modified after creation of enumerator");
    }
    public void Dispose()
    {
    }

    public bool MoveNext()
    {
        ThrowOnModification();
        if (_index + 1 == _count)
            return false;
        _index++;
        return true;
    }

    public void Reset()
    {
        ThrowOnModification();
        _index = -1;
    }

    object IEnumerator.Current
    {
        get { return Current; }
    }

    public T Current
    {
        get { return _list[_index]; }
    }

    public void Remove()
    {
        ThrowOnModification();
        _list.RemoveAt(_index);
        _index--;
        _count--;
    }
}
Evgeniy Berezovsky
  • 18,571
  • 13
  • 82
  • 156
1

There is a good discussion of this in Removing items in a list while iterating through it .

They propose:

for(int i = 0; i < count; i++)
{
    int elementToRemove = list.Find(<Predicate to find the element>);

    list.Remove(elementToRemove);
}
Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
Ethan Heilman
  • 16,347
  • 11
  • 61
  • 88
  • A little overly general (and it needs a check for a failed Find) – BCS May 06 '09 at 23:40
  • 1
    @BCS Only if the list type is non-nullable, neither Find or Remove break on failure, (though calling Remove when the item isn't there is unnecessarily expensive) – rmoore Jul 03 '09 at 16:42
1

If you know your list isn't very large you can use

foreach (Type o in new List<Type>(Os))
    ....

which will create a temporary duplicate of the list. Your remove() call will then not be interfering with the iterator.

Nik
  • 1,364
  • 8
  • 7
  • O(n) copy + O(n^2) removes. (Remove(T) is O(n) and I'd do up to n of them) – BCS May 06 '09 at 22:56
  • Yeah repeated calls to remove() will never win any awards for performance (and will totally dwarf the list copy cost), though it would 'look' similar to how you want it (as you asked). RemoveRange() is your best bet for performance. – Nik May 06 '09 at 23:08
1

Look at Enumerable.SkipWhile()

Enumerable.SkipWhile( x => condition).ToList()

Generally not mutating a list, makes live a lot easier. :)

leppie
  • 115,091
  • 17
  • 196
  • 297
0

I just had the same problem and solved it by using the following:

foreach (Type o in (new List(Os))) { if (something) Os.Remove(o); }

It iterates through a copy of the list and removes from the original list.

Christiaan Maks
  • 3,248
  • 3
  • 23
  • 27
0

Add the item to remove in a list, and then remove these items by using RemoveAll:

List<Type> Os;
List<Type> OsToRemove=new List<Type>();
...
foreach (Type o in Os){
    if (o.cond)
        return;
    else
        OsToRemove.Add(o);
}
Os.RemoveAll(o => OsToRemove.Contains(o));
Wei Jian
  • 269
  • 3
  • 4