Remove Single Element From Collection in Foreach

Question

Was just wondering if it is unsafe / bad form / just kind of looks dirty to remove a single element from a Collection while in a foreach loop without saving it to do outside the loop. I know if you had to keep iterating, it could break the loop (and in some cases, just throw an error), but in this case you do not need to iterate again because you are using a break statement when you find what you are looking for.

Example:

List<string> strings = new List<string>() { "A", "B", "C" };
foreach(string s in strings)
{
    if (s == "B")
    {
        strings.Remove("B");
        break;
    }
}

Is this bad? I would normally be writing this as either a backwards loop or caching the stringToRemove in a variable (with the obvious name stringToRemove), but just wondered how something like this felt to other folks.

while this will not error out, it is still a code smell – Steve Sep 10 '18 at 19:55 — Steve, Sep 10 '18 at 19:55

score 3 · Answer 1 · answered Sep 10 '18 at 18:36

3

Instead of using a foreach loop and having somewhat messy/hacky code. Just one line it with the RemoveAll() method:

strings.RemoveAll(x => x == "B");

Fiddle here

answered Sep 10 '18 at 18:36

maccettura

10,514
3
28
35

score 0 · Answer 2 · answered Sep 11 '18 at 04:11

You can't delete with foreach because foreach uses yield keyword which doesn't allow to modify Why?.

maccettura's answer is alright but if you care about performance, don't use LINQ in this case because LINQ basically will iterate entire list no matter what, you just want to remove the first value so just use the old style - for or while. Your code is not good because you are using "==" to compare strings.

Your code might looks like this:

Solution 1: for loop is slower than foreach, this approach is for simplicity.

List<string> strings = new List<string>() { "A", "B", "C" };
for(int i=0;i<=strings.Count;i++)
{
    if (strings[i].Equals("B")) //don't use == to compare string in C#
    {
        strings.RemoveAt(i);
        break;
    }
}

Solution 2: better performance, might be complicate

List<string> strings = new List<string>() { "A", "B", "C" };
int deletePosition=0; 
bool isFound=false;
foreach(string s in strings)
{
    deletePosition++;
    if (s.Equals("B")) //don't use "==" to compare string
    {
        isFound=true;
        break;
    }
}

if(deletePosition<strings.Count-- && isFound)
{
    strings.RemoveAt(deletePosition);
}

Remove Single Element From Collection in Foreach

2 Answers2