views:

1244

answers:

6

I have two ListBoxes, lstAvailableColors and lstSelectedColors. Between each listbox are two buttons, Add and Remove. When a color or colors is selected in lstAvailableColors and the Add button is clicked, I want to remove them from lstAvailableColors and display them in lstSelectedColors. Also, if colors are selected in lstSelectedColors and the Remove button is clicked, I want to remove the colors from lstSelectedColors and add them back to lstAvailableColors. When I do this, I get the following error when it removes the item:

Collection was modified; enumeration operation may not execute.

Here is the code for the Add Button and the Remove Button:

Add:

    protected void btnAdd_Click(object sender, EventArgs e)
    {
        foreach (ListItem item in lstAvailableColors.Items)
        {
            if (item.Selected)
            {
                lstSelectedColors.Items.Add(item);
                lstAvailableColors.Items.Remove(item);
            }
        }
    }

Remove:

    protected void btnRemove_Click(object sender, EventArgs e)
    {
        foreach (ListItem item in lstSelectedColors.Items)
        {
            if (item.Selected)
            {
                lstAvailableColors.Items.Add(item);
                lstSelectedColors.Items.Remove(item);
            }
        }
    }
+1  A: 

You can't modify a collection while you're iterating over it. Either iterate over a copy or use for, iterate in reverse and remove as you go down.

Arnshea
Do you have an example?
Xaisoft
for (int i=lstSelectedColors.Items.Count-1; i >= 0; i--) { if ( lstSelectedColors.Items[i].Selected] ) { lstSelectedColors.Items.Remove(lstSelectedColors.Items[i]); } }
Arnshea
You contradict yourself; you say you cannot modify a collection while you are iterating over it (no direction specified), but you immediately say that the solution is to iterate in reverse. It's just generally a bad idea to modify a collection you're iterating over, forward or reverse.
McWafflestix
I was using iterate too loosely. Foreach enumerates over a collection. You can't modify it while enumerating over the collection. But you can modify it by using a for loop and starting at the end of the collection.
Arnshea
I contend that it is still a bad idea to modify a collection while iterating over it (even using a for loop). It may work, but it's generally bad form.
McWafflestix
agreed, though I'm curious - why do you say it's bad form? What's bad about it?
Arnshea
+4  A: 

You cannot modify an collection while you are using an Enumerator for this collection, what the for each statement does.

You have to loop over the data with a normal for loop and then you can modify the collection, but you must be careful to correctly update the current index if you insert or remove elements. If you just add or remove elements and don't insert some, iterating from the last element to the first will do.

protected void btnAdd_Click(object sender, EventArgs e)
{
    for (Int32 i = lstAvailableColors.Items.Count; i >= 0; i--)
    {
        ListItem item = lstAvailableColors.Items[i];

        if (item.Selected)
        {
            lstSelectedColors.Items.Add(item);
            lstAvailableColors.Items.Remove(item);
        }
    }
}
Daniel Brückner
+5  A: 

It's not possible to modify a collection while you're enumerating it in .Net. You need to separate out your enumeration and remove code into different blocks. Here is a quick sample on how to do that in with LINQ

 protected void btnAdd_Click(object sender, EventArgs e)
    {
        var selected = new List<ListItem>();
        foreach (ListItem item in lstAvailableColors.Items)
        {
            if (item.Selected)
            {
                selected.Add(item);
                lstSelectedColors.Items.Add(item);
            }
        }
        foreach (ListItem item in selected)
        {
            lstAvailableColors.Items.Remove(item);
        }
    }

And here's a more concise version using LINQ

  var selected = lstAvailableColors.Cast<ListItem>().Where(i => i.Selected).ToList();
  selected.ForEach( x => { lstSelectedColors.Items.Add(x); });
  selected.ForEach( x => { lstAvailableColors.Items.Remove(x);});

EDIT

The LINQ version works in two parts. The first part is the first line which finds the currently selected items and stores the value in a List<ListItem>. It's very important that the line contain the .ToList() call because that forces the query to execute immediately vs. being delayed executed.

The next two lines iterate through each value which is selected and remove or add it to the appropriate list. Because the selected list is already stored we are no longer enumerating the collection when we modify it.

JaredPar
Can you explain the LINQ version?
Xaisoft
"with LINQ" should be "without LINQ" in the first example ;)
Lucas
+3  A: 

As the other answer mentioned, you can't remove items until you've completed the iteration. So perhaps something like this will be cleanest for you:

var itemsToRemove =
lstAvailableColors.Items.Cast<ListItem>().Where(i => i.IsSelected).ToArray();

foreach(ListItem item in itemsToRemove) lstAvailableColors.Remove(item);
mquander
+3  A: 

You cannot modify a collection you are iterating on. In general, a good solution for this type of problem is to create an empty collection, and in your iterator, copy over all of the elements you do NOT want to remove; after the iteration is complete, replace the original collection with your new collection.

McWafflestix
A: 

Example on how to remove the selected Items. Here only the selected indices are taken and removed.

   public void RemoveSelectedItems(ListBox listbox)
   {
       List<ListItem> items = GetSelectedItems(listbox);
       foreach (var listItem in items)
       {
           listbox.Items.Remove(listItem);
       }
   }

  public List<ListItem> GetSelectedItems(ListBox listbox)
  {
     int[] selectedIndices = listbox.GetSelectedIndices();
     return selectedIndices.Select(index => listbox.Items[index]).ToList();
  }
dhinesh