Overloaded constructors problem

[Premier2k]

I've just been musing over a problem I had and the solution and I thought of another problem that I would encounter, so I checked it and it is a problem....

After setting territories and continents, the user enters a borders screen.
When the form loads it populates a drop down box with the territory names that it gets from collection and a listbox (called lbUnassigned) with the same information. The user selects a territory from the drop down box and then proceeds to add territories from the listbox to another list box (lbBorder). When the user presses SAVE then the territory is removed from the dropdown and the two list boxes are reset, i.e lbBorders is empty and lbUnassigned is cleared and repopulated with the list of territories.

Except that when the user presses SAVE, the territory disappears from the dropdown (correct) and the same territory is added AGAIN to the lbUnassigned box (wrong) I think what's happening is another example of incorrect usage of overloaded constructors.

This is my class that holds the collection:
class Territories
{
string tname;
int sx, sy, lx, ly, neutVal;
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 int NeutVal
{
get { return this.neutVal; }
set { this.neutVal = 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, int sx, int sy, int lx, int ly, bool cbS, bool cbK, bool cbN, bool cbB, int neutVal)
{
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;
this.NeutVal = neutVal;
}

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;
}
So, as you can see I am again using tname when assigning the borders and bombards:
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;
}
}
Now later on, I need to be able to loop through my collection and look at tname and then find out what borders it has, but as far as I'm aware, the collection doesn't know any relationship between them. So removing the string tname from the method would just give me a list of borders in an array wouldn't it? How do I know which borders were allocated to which territory?

Premier2k

[BigEd781]

first off, I would recommend chaining your constructors so that your code is more maintainable. I would also recommend packing those multiple arguments into an object or two instead of 10+ single parameters, but that is another issue altogether:


public Territories(string tname, int sx, int sy, int lx, int ly, bool cbS, bool cbK, bool cbN, bool cbB)
: this( tname, sx, sy, lx, ly, cbS, cbK, cbN, cbB, 0 )
{

}

public Territories(string tname, int sx, int sy, int lx, int ly, bool cbS, bool cbK, bool cbN, bool cbB, int neutVal)
{
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;
this.NeutVal = neutVal;
}


This approach eliminates the duplicate code in both constructors.

Also, this constructor has some problems:

public Territories(string tname, ArrayList list, ArrayListType type)


I would really recommend using a List<T> instead of an Arraylist. The ArrayList class was used back in the days of .NET 1.1 and really has no purpose now that we have generics. Also, it is hard for me to understand why you would use this constructor. Almost all of your properties will be null, false, or 0, and at least one of the two lists will always be null, so now you must always check for null before using them ( as will clients of this code because the lists are public). Also, you should not be simply exposing them as public variables. Expose them in a property as an interface (i.e., IList<T>) or a ReadOnlyCollection instead. Also, I would not be a fan of maintaining code which uses an enum to decide which collection to initialize.

Now, with that out of the way, I can say that... I can't really help you with your main problem without seeing more code :). I think that we need to see the code which runs when the Save button is clicked.

[Premier2k]

This is my code that stores the borders to the selected territory.
private void btnSave_Click(object sender, RoutedEventArgs e)
{
if (lbBorders.Items.Count == 0)
{
if (System.Windows.MessageBox.Show("Are you sure you wish to assign 0 borders to this territory", "Confirm", MessageBoxButton.YesNo, MessageBoxImage.Question) == MessageBoxResult.Yes)
{
saveTerrBorders();
}
}
else
{
try
{
cname = ddsterr.Text.ToString();
foreach (object x in lbBorders.Items)
{
bTer.Add(x.ToString());
lbUnassigned.Items.Add(x);
}
gbl_lisjt.lijst.Add(new Territories(bTer, Territories.ArrayListType.border));
ddsterr.Items.Remove(cname);
lbSaved.Items.Add(cname);
lbBorders.Items.Clear();
}
catch (FormatException fEx)
{
System.Windows.MessageBox.Show("Please enter a value into all boxes", "Error");
logit.WriteToLog(@"c:\logfile.log", fEx.ToString());
}
catch (Exception err)
{
System.Windows.MessageBox.Show("Handled exception, check log for more details", "Error");
logit.WriteToLog(@"c:\logfile.log", err.ToString());
}
}
}

and this is my code that stores the territories initially:
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();
}
else
return;
}
}
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);
if (tbneutralValue.IsEnabled == true)
{
neutVal = int.Parse(tbneutralValue.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, neutVal));
}
else
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, false, true, true, neutVal));
}
}
else if (cbKiller.IsChecked == true)
{
if (cbNeutral.IsChecked == true)
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, true, true, true, neutVal));
}
else
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, false, true, true, neutVal));
}
}
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, neutVal));
}
else
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, false, true, false, neutVal));
}
}
else if (cbKiller.IsChecked == true)
{
if (cbNeutral.IsChecked == true)
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, true, true, false, neutVal));
}
else
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, false, true, false, neutVal));
}
}
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 and ensure that you have entered a value in neutral start", "Error");
}
// Catch everything else
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();
}
}

[JonnyPoet]

Excuse my question but maybe I cannot get smething right or the design isn't that way I'm normally using in such case.

You have a class of territories. I cannot see the class of a single terretory which would be collected i the terretories collection class.
When I have to do something where I choose objects by use of lists or comboboxes I normally would have a class who has overridden its ToString method so the name, in your name the terretory name is used for classes ToString() method. This way when I add the class to e.g. a combobox as an object the list automatically would show the classname and in the SelectedItem you always can retrieve the class by use of selectedItem as Terretory.
Each Terretory has its own properties which would reduce the amount of parameters in your Terretories class, when I have got the idea. and retrieving the teretory would also maintain you with its borders. Or have I totally misuplicated your aproach.

[JonnyPoet]

This of your code

// 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, neutVal));
}
else
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, false, true, true, neutVal));
}
}
else if (cbKiller.IsChecked == true)
{
if (cbNeutral.IsChecked == true)
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, true, true, true, neutVal));
}
else
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, false, true, true, neutVal));
}
}
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, neutVal));
}
else
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, false, true, false, neutVal));
}
}
else if (cbKiller.IsChecked == true)
{
if (cbNeutral.IsChecked == true)
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, true, true, false, neutVal));
}
else
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, false, true, false, neutVal));
}
}
else
{
gbl_lisjt.lijst.Add(new Territories(tname, sx, sy, lx, ly, false, false, false, false));
}
}
#endregion
}

Could be replaced by the following IMHO

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