Problem with collections

[Premier2k]

Hi all,

When the user clicks on an item in a listbox and clicks edit, it displays the contents of the items collection in the various fields, it is then removed from the listbox and the collection.
The user can make any changes and click add and the item is restored back into the collection. However, I am getting some errors!

This is my Edit method:

private void btnEditSelected_Click(object sender, RoutedEventArgs e)
{
getSelected = lbnames.SelectedItem.ToString();
foreach (Territories oTerritory in gbl_lisjt.lijst)
{
correctName = oTerritory.Tname.ToString();
if (correctName.Equals(getSelected) == true)
{
tbName.Text = getSelected;
tbsmallx.Text = oTerritory.Sx.ToString();
tbsmally.Text = oTerritory.Sy.ToString();
tblargex.Text = oTerritory.Lx.ToString();
tblargey.Text = oTerritory.Ly.ToString();
if (oTerritory.CbS == true)
{
cbStarting.IsChecked = true;
}
if (oTerritory.CbN == true)
{
cbNeutral.IsChecked = true;
}
if (oTerritory.CbK == true)
{
cbKiller.IsChecked = true;
}
if (oTerritory.CbB == true)
{
cbBombard.IsChecked = true;
}
gbl_lisjt.lijst.RemoveAll(d => d.Tname == lbnames.SelectedValue.ToString());
}
}
}

My Add method:

private void btnAdd_Click(object sender, RoutedEventArgs e)
{
if (tbName.Text == "")
{
System.Windows.MessageBox.Show("Please enter a territory name", "Error");
tbName.Focus();
}
else
{
tname = tbName.Text;
foreach (Territories oCheck in gbl_lisjt.lijst)
{
sReturned = oCheck.Tname.ToString();
if (sReturned.Equals(tname))
{
if (System.Windows.MessageBox.Show("You have identical territory names. Do you wish to overwrite the stored name?", "Confirm", MessageBoxButton.YesNo, MessageBoxImage.Question) == MessageBoxResult.Yes)
{
overwrite();
}
}
}
try
{
// Attempt to parse the numbers, encapsulated in a Try/Catch in case letters are entered
sx = int.Parse(tbsmallx.Text);
sy = int.Parse(tbsmally.Text);
lx = int.Parse(tblargex.Text);
ly = int.Parse(tblargey.Text);

#region IF statement covering the possible selection outcomes
// May rewrite this as a SWITCH statement later.
if (cbBombard.IsChecked == true)
{
if (cbStarting.IsChecked == true)
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, true, false, false, true));
}
else if (cbNeutral.IsChecked == true)
{
if (cbKiller.IsChecked == true)
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, true, true, true));
}
else
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, false, true, true));
}
}
else if (cbKiller.IsChecked == true)
{
if (cbNeutral.IsChecked == true)
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, true, true, true));
}
else
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, false, true, true));
}
}
else
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, false, false, true));
}
}
else
{
if (cbStarting.IsChecked == true)
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, true, false, false, false));
}
else if (cbNeutral.IsChecked == true)
{
if (cbKiller.IsChecked == true)
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, true, true, false));
}
else
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, false, true, false));
}
}
else if (cbKiller.IsChecked == true)
{
if (cbNeutral.IsChecked == true)
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, true, true, false));
}
else
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, false, true, false));
}
}
else
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, false, false, false));
}
}
#endregion
}

// In case user enters a non valid character into the x/y field
catch (FormatException fEx)
{
logit.WriteToLog(@"c:\logfile.log", fEx.ToString());
System.Windows.MessageBox.Show("Please enter numbers into the x/y co-ords fields", "Error");
}
// Catch everything
catch (Exception err)
{
logit.WriteToLog(@"c:\logfile.log", err.ToString());
System.Windows.MessageBox.Show("Something went wrong, please PM Premier2k for his email address");
}
lbnames.Items.Add(tbName.Text);
reset();
}
}

and the class that stores my collection:

class Territories
{
string tname;
int sx, sy, lx, ly;
bool cbS, cbK, cbN, cbB;
public ArrayList aBorder, aBombard;
public enum ArrayListType { border = 1, bombard = 2 }

public string Tname
{
get { return this.tname; }
set { this.tname = value; }
}

public int Sx
{
get { return this.sx; }
set { this.sx = value; }
}

public int Sy
{
get { return this.sy; }
set { this.sy = value; }
}

public int Lx
{
get { return this.lx; }
set { this.lx = value; }
}

public int Ly
{
get { return this.ly; }
set { this.ly = value; }
}

public bool CbS
{
get { return this.cbS; }
set { this.cbS = value; }
}

public bool CbK
{
get { return this.cbK; }
set { this.cbK = value; }
}

public bool CbN
{
get { return this.cbN; }
set { this.cbN = value; }
}

public bool CbB
{
get { return this.cbB; }
set { this.cbB = value; }
}

public Territories(string tname, int sx, int sy, int lx, int ly, bool cbS, bool cbK, bool cbN, bool cbB)
{
this.Tname = tname;
this.Sx = sx;
this.Sy = sy;
this.Lx = lx;
this.Ly = ly;
this.CbS = cbS;
this.CbK = cbK;
this.CbN = cbN;
this.CbB = cbB;
}

public Territories(string tname, ArrayList list, ArrayListType type)
{
this.tname = tname;

if (type == ArrayListType.border)
{
this.aBorder = list;
}
else if (type == ArrayListType.bombard)
{
this.aBombard = list;
}
}
}

static class gbl_lisjt
{
static gbl_lisjt()
{
Debug.WriteLine("** creating application scope instance **");
lijst = new List<Territories> ();
}

static public List<Territories> lijst;
}


Now when I click an item in the listbox and click Edit I get this exception error message: Collection was modified; enumeration operation may not execute.
The error occurs in the Edit method on this line:

foreach (Territories oTerritory in gbl_lisjt.lijst)
and highlights the word 'in'


What does that mean? I checked the help but that means less to me than the message itself!

Premier2k

[BigEd781]

You cannot modify a collection while iterating through it. For example, consider this code:


foreach ( string s in someCollection )
{
someCollection.Remove( s );
}


So, how does the iterator continue? You have changed the collection that the iterator was based on, it's state has been changed. If you change that code to this you will not get an exception, but it will also not do what you think it will:


for ( int i = 0; i < str.Count; ++i )
{
str.Remove( str[i] );
}


That will not remove every element from str, it will remove three elements and miss two of them because "Count" changes as you remove items. So, what I usually do is add each item you want to remove to a separate collection and then iterate through that and remove the items from the first, or, simply iterate backwards through the collection using a for loop.


for ( int i = str.Count - 1; i >= 0; --i )
{
str.Remove( str[i] );
}

[Shuja Ali]

The error message is pretty descriptive and does tell you where the problem is. Imagine you are driving a car and someone just removes one of the wheels from it, how would the car respond? IN a similar manner, you are trying to remove an item from collection while you are enumerating through it. The ideal solution would be not to remove the items in the same loop. You just select the items (save them in an array our another temporary collection) that need to be removed and then use another foreach loop to remove them from the collection.

[monalin]

You cannot edit the list you're iterating through inside of a foreach loop. So this line here is whats causing the error. It's dangerous to do this.

gbl_lisjt.lijst.RemoveAll(d => d.Tname == lbnames.SelectedValue.ToString());

Its possible to work around this but I never suggesting removing items from a list when you're inside the loop iterating through that list.

One thing you can do is create a copy of that list and iterate through the copy and only remove them from the master list. This will stop the error but I don't recommend it.


foreach(Territories oTerritory in gbl_lisjt.lijst.ToList())


However, its never a good idea to remove items in a list when you are iterating through that list. There must be some other way to redesign your code so this doesn't happen. Rewrite the edit function like this, and you shouldn't get any errors.


private void btnEditSelected_Click(object sender, RoutedEventArgs e)
{
String getSelected = lbnames.SelectedItem.ToString();
List<Territories> matches = gbl_lisjt.lijst.FindAll(name => name == getSelected);
foreach (Territories oTerritory in matches)
{
tbName.Text = getSelected;
tbsmallx.Text = oTerritory.Sx.ToString();
tbsmally.Text = oTerritory.Sy.ToString();
tblargex.Text = oTerritory.Lx.ToString();
tblargey.Text = oTerritory.Ly.ToString();

if (oTerritory.CbS)
cbStarting.IsChecked = true;
if (oTerritory.CbN)
cbNeutral.IsChecked = true;
if (oTerritory.CbK)
cbKiller.IsChecked = true;
if (oTerritory.CbB)
cbBombard.IsChecked = true;

gbl_lisjt.lijst.RemoveAll(d => d.Tname == lbnames.SelectedValue.ToString());
}
}


Notice how I selected only the objects that I needed to work with and then iterated through those. This allows me to remove anything I want from the original list because i'm not longer iterating through that list.

[Premier2k]

Thanks guys!
I have modified my code and I now have this:

private void btnEditSelected_Click(object sender, RoutedEventArgs e)
{
getSelected = lbnames.SelectedItem.ToString();

for (int i = gbl_lisjt.lijst.Count - 1; i >= 0; i--)
{
Territories oTerritory = gbl_lisjt.lijst[i];

correctName = oTerritory.Tname.ToString();
if (correctName.Equals(getSelected) == true)
{
tbName.Text = getSelected;
tbsmallx.Text = oTerritory.Sx.ToString();
tbsmally.Text = oTerritory.Sy.ToString();
tblargex.Text = oTerritory.Lx.ToString();
tblargey.Text = oTerritory.Ly.ToString();
if (oTerritory.CbS == true)
{
cbStarting.IsChecked = true;
}
else
cbStarting.IsChecked = false;
if (oTerritory.CbN == true)
{
cbNeutral.IsChecked = true;
}
else
cbNeutral.IsChecked = false;
if (oTerritory.CbK == true)
{
cbKiller.IsChecked = true;
}
else
cbKiller.IsChecked = false;
if (oTerritory.CbB == true)
{
cbBombard.IsChecked = true;
}
else
cbBombard.IsChecked = false;
}

if (oTerritory.Tname == getSelected)
{
gbl_lisjt.lijst.RemoveAt(i);
lbnames.Items.RemoveAt(lbnames.SelectedIndex);
System.Diagnostics.Debug.WriteLine("Territories collection(lijst) now contains " + dbgCount.ToString() + " items");
}
}
}

This seems to work fine!

Premier2k

[monalin]

A side note.


if (cbBombard.IsChecked == true)
{
if (cbStarting.IsChecked == true)
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, true, false, false, true));
}
else if (cbNeutral.IsChecked == true)
{
if (cbKiller.IsChecked == true)
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, true, true, true));
}
else
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, false, true, true));
}
}
else if (cbKiller.IsChecked == true)
{
if (cbNeutral.IsChecked == true)
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, true, true, true));
}
else
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, false, true, true));
}
}
else
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, false, false, true));
}
}
else
{
if (cbStarting.IsChecked == true)
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, true, false, false, false));
}
else if (cbNeutral.IsChecked == true)
{
if (cbKiller.IsChecked == true)
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, true, true, false));
}
else
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, false, true, false));
}
}
else if (cbKiller.IsChecked == true)
{
if (cbNeutral.IsChecked == true)
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, true, true, false));
}
else
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, false, true, false));
}
}
else
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, false, false, false));
}
}


Can be re-written in one line. At least i think it can if I understand what you're trying to do. Just a thought.


gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, cbStarting.IsChecked, cbKiller.IsChecked, cbNeutral.IsChecked, cbBombard.IsChecked));


Before you copy and past code, take a second to see if you can find some way to do it without copying and pasting. Anytime i catch myself doing this i change my code, it makes it soooo much easier to read. Instead of it takeing me 4 minutes to understand what you're trying to type it takes 2 seconds. Try it, you'll see what i mean.