<< Programmer Symbology | Home | Health Monitoring and ASP.NET MVC >>

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

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. 

Print | posted @ Sunday, October 11, 2009 9:12 PM

Comments on this entry:

Gravatar # re: What’s Wrong With This Code? (#22)
by Erik at 10/11/2009 11:32 PM
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 # re: What’s Wrong With This Code? (#22)
by daub815 at 10/11/2009 11:37 PM
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
  
Gravatar # re: What’s Wrong With This Code? (#22)
by diryboy at 10/11/2009 11:53 PM
but why not this?
cities.RemoveAll(c => c.Length < 7);
  
Gravatar # re: What’s Wrong With This Code? (#22)
by Mike at 10/12/2009 12:14 AM
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 # re: What’s Wrong With This Code? (#22)
by Gabriel Lozano-Moran at 10/12/2009 1:36 AM
It has got to do with the deferred execution nature of LINQ and can easily be solved by using the ToList operator.
  
Gravatar # re: What’s Wrong With This Code? (#22)
by xhinker at 10/12/2009 7:57 AM
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 # re: What’s Wrong With This Code? (#22)
by Amit at 10/12/2009 9:09 AM
without changing much, I would say modify foreach as follows

foreach (var s in citiesToRemove.ToList())
  
Gravatar # re: What’s Wrong With This Code? (#22)
by Nick Berardi at 10/12/2009 9:18 AM
var citiesToKeep = cities.Where(city => city.Length >= 7);
  
Gravatar # re: What’s Wrong With This Code? (#22)
by scott at 10/12/2009 9:51 AM
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 # re: What’s Wrong With This Code? (#22)
by Eber Irigoyen at 10/12/2009 4:08 PM
as mentioned earlier, cities.RemoveAll(c => c.Length < 7); will replace all those lines
  
Gravatar # re: What’s Wrong With This Code? (#22)
by Scott Allen at 10/12/2009 4:15 PM
@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!
  
Gravatar # re: What’s Wrong With This Code? (#22)
by dude at 10/13/2009 5:43 AM
foreach (var s in citiesToRemove.ToArray())
{
cities.Remove(s);
}
  
Gravatar # re: What’s Wrong With This Code? (#22)
by Mike at 10/13/2009 8:42 AM
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 have been closed on this topic.
Scott Allen
Posts - 869
Comments - 4493
Stories - 14