What’s Wrong With This Code? (#22)

Sunday, October 11, 2009

It’s a bug you’ve probably learned to avoid in .NET programming, it’s just not as obvious now. 

var cities = new List<string>
{ 
    "Baltimore", 
    "Munich", 
    "Copenhagen" 
};

var citiesToRemove = 
    cities.Where(city => city.Length < 7);

foreach (var s in citiesToRemove)
{
    cities.Remove(s);
}
What goes wrong, and what’s an easy fix?

--

All links to my “What’s Wrong” series are here. 

Comments
gravatar Erik Sunday, October 11, 2009
I'd have to say, modifying a collection whilst enumerating it.

citiesToRemove is being lazily evaluated in the foreach, and thus has an enumerator for cities open. The easy fix is to call .ToList() after the call to .Where() when initializing citiesToRemove.
gravatar daub815 Sunday, October 11, 2009
I wouldn't use a foreach when you want to remove items. The C# documentation also suggests not to.

msdn.microsoft.com/en-us/library/ttw7t8t6.aspx
diryboy Sunday, October 11, 2009
but why not this?
cities.RemoveAll(c => c.Length < 7);
gravatar Mike Monday, October 12, 2009
Rather than a foreach run through the list in reverse index order. Once you remove a member you won't be stepping past the end of the collection
gravatar Gabriel Lozano-Moran Monday, October 12, 2009
It has got to do with the deferred execution nature of LINQ and can easily be solved by using the ToList operator.
gravatar xhinker Monday, October 12, 2009
use for other than foreach if we want to change the content of collection within a loop.
I remembered that "Effective C#" discussed this topic.
gravatar Amit Monday, October 12, 2009
without changing much, I would say modify foreach as follows

foreach (var s in citiesToRemove.ToList())
gravatar Nick Berardi Monday, October 12, 2009
var citiesToKeep = cities.Where(city => city.Length >= 7);
gravatar scott Monday, October 12, 2009
Congrats to everyone who mentioned .ToList(). Without ToList you'll be modifying the collection the code is enumerating, which generates an exception.

I think it's tricky because it looks like citiesToRemove is a different collection...
gravatar Eber Irigoyen Monday, October 12, 2009
as mentioned earlier, cities.RemoveAll(c => c.Length < 7); will replace all those lines
gravatar Scott Allen Monday, October 12, 2009
@Eber: Yes - you are absolutely correct.

The List's RemoveAll takes a predicate and would make a fix that would also take a lot less code.

Sorry I didn't call that out earlier!
dude Tuesday, October 13, 2009
foreach (var s in citiesToRemove.ToArray())
{
cities.Remove(s);
}
gravatar Mike Tuesday, October 13, 2009
dude has it right. You can't modify a collection as you are iterating over it.

By converting it to an array or any other IEnumerable before removing an element this issue is resolved.
Comments are now closed.
Follow Me On Twitter
RSS Subscribe
Contact
Search Archives About
OdeToCode by K. Scott Allen
K.Scott Allen