CODEWITHSUNDEEP
c#collectionssyntax
Chad Stewart
Chad Stewart

Reputation: 484

Remove Single Element From Collection in Foreach

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.

Upvotes: 0

Views: 416

Answers (2)

Red Wei
Red Wei

Reputation: 942

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);
}

Upvotes: 0

maccettura
maccettura

Reputation: 10818

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

Upvotes: 3

Related Questions