Code won't exit foreach block

Question

I've got the following C# code segment that takes a list, finds objects that are ready to update, then shoves them into a temp list, deletes from the main list, and then goes on its merry way. My issue is that the foreach block, which cycles through my main list, won't exit.

 TempLog.Clear();   //Ensure TempLog is empty
 foreach (CLogger ready in PlayerLog)
 {
      if (ready.UpdateReady == true)  // Record is ready to be updated in database
      {
           TempLog.Add(ready);  // Add record to templog
           PlayerLog.Remove(ready);  // Remove from playerlog
      }
 }
              <----  Never reaches this point
 if (TempLog.Count > 0)  // Just check that templog isn't empty
 {
      new Thread(Update).Start();  // Run update code 
 }

I've put heaps of debugging in, and I can watch PlayerLog start at 1, TempLog at 0, then it enters the foreach loop, picks up that the record UpdateReady flag is on, TempLog goes to 1, PlayerLog goes to 0, then it just stops.. No errors, just stops..

Thanks for the help :)

Answer 1

You're modifying PlayerLog as you iterate over its enumerator, which is a big no-no.

A better option would be to iterate over a copy of PlayerLog, while modifying the original PlayerLog:

foreach (CLogger ready in new List<PlayerLogType>(PlayerLog))
 {
      if (ready.UpdateReady)  // Please don't compare boolean values against true...
      {
           TempLog.Add(ready);
           PlayerLog.Remove(ready);
      }
 }

Alternatively, you could use a traditional for-loop. Personally, I like to start from the end when I know I'll be removing items, so I don't have to readjust the index:

for (int i = PlayerLog.Count - 1; i >= 0; i--)
{
      var ready = PlayerLog[i];
      if (ready.UpdateReady)
      {
           TempLog.Add(ready);
           PlayerLog.RemoveAt(i);
      }
}

Answer 2

As Mark pointed out, a foreach loop often breaks when you modify the collection you're looping over.

In this case, you may be able to avoid making a copy of the list, since you have a temporary list to use. If the final contents of TempLog don't matter, try this:

 TempLog.Clear();   // initial "new" list
 foreach (CLogger ready in PlayerLog)
 {
      if (ready.UpdateReady != true)
           TempLog.Add(ready); // item being kept (not removed) - add to "new" list
 }

 if (TempLog.Count != PlayerLog.Count)  // check if any items were not kept
 {
      PlayerLog.Clear();
      PlayerLog.AddRange(TempLog);
      new Thread(Update).Start();  // Run update code 
 }

Another solution would be to store the list of items to be removed in a queue or stack, then remove them all after the foreach loop.

Answer 3

Odd, it should just throw an exception when you try to change the collection you're iterating like that. I'd do this:

var ready = PlayerLog.Where(c => c.UpdateReady).ToArray();
if (ready.Any())
{
    TempLog.Clear();  //Ensure TempLog is empty
    TempLog.AddRange(ready);
    foreach (CLogger c in ready) PlayerLog.Remove(c);
    new Thread(Update).Start();  // Run update code 
}       

Also, this looks a little like you're trying to build a producer/consumer queue. If so, you should look at Microsoft's Concurrency and Coordination Runtime, which oddly right now is buried inside Microsoft Robotics Studio.

Answer 4

Here's a simple method with lambda expressions:

TempLog.Clear();
TempLog.AddRange(PlayerLog.Where(a => a.UpdateReady));
TempLog.ForEach(a => PlayerLog.Remove(a));

Answer 5

Mark is right, but use the for loop so you don't waste memory.