Another question about improving the code

[Tony Johansson]

Hello!

This Add method below is called when I want to add an animal to the animal
collection that is located in the AnimalManager class.In the ValidateData
method that is called from the Add method is used for checking some business
rules.
In this method ValidateData I use the GetType to get the dynamic type but
somebody of you mentioned that it was not appropriate to do in the way that
I now do but instead should use a property. It was especially Peter and Arne
that was mentioned this and I hope they will read this thread.
This animal parameter that is passed to this Add method is comming from a
selection in a ListBox in the GUI class.
So I just want to know how the implementation of this property should look
like.

animalManager.Add(category, animal, txtName.Text,
double.Parse(txtAge.Text.Trim()), gender, housing, eater, false,
foodMenuList);

public void Add(CategoryType category, string animal, string name, double
age, GenderType gender,HousingType housing,
EaterType eater, bool aggressive, List<string>
foodMenuList)
{
//Check some business rules
ValidateData(name, animal);

//Generate the ID that is unique each animal in the collection
string ID = GenerateID();

switch (category)
{
case CategoryType.Mammal:
MammalSpices mammalSpecies =
(MammalSpices)Enum.Parse(typeof(MammalSpices), animal);
animals.Add( (Animal)
MammalFactory.CreateMammal(mammalSpecies, name,ID, age, gender, housing,
eater, false, foodMenuList));
break;

case CategoryType.Bird:
BirdSpices
birdSpecies=(BirdSpices)Enum.Parse(typeof(BirdSpices),animal);
animals.Add((Animal)BirdFactory.CreateBird(birdSpecies, name,
ID, age,gender, housing, eater, false,
foodMenuList));
break;

case CategoryType.Insect:
InsectSpecies insectSpecies =
(InsectSpecies)Enum.Parse(typeof(InsectSpecies),animal);
animals.Add((Animal)InsectSpecies.CreateBird(insectSpecies,
name, ID, age, gender, housing, eater, false,
foodMenuList));
break;

case CategoryType.Marine :
MarineSpices marineSpecies =
(MarineSpices)Enum.Parse(typeof(MarineSpices), animal);
animals.Add((Animal)MarineSpices.CreateBird(marineSpecies,
name, ID, age, gender, housing, eater, false,
foodMenuList));
break;

case CategoryType.Reptile :
ReptileSpecies reptileSpecies =
(ReptileSpecies)Enum.Parse(typeof(ReptileSpecies), animal);
animals.Add((Animal)ReptileSpecies.CreateBird(reptileSpecies,
name, ID, age, gender, housing, eater, false,
foodMenuList));
break;
}
}

//Check the business rules that say there must not be two names for the same
animalType
private void ValidateData(string name, string animal)
{
Animal thisAnimal = (Animal)animals.Find(delegate(Animal item)
{
return (string.Equals(item.Name, name,
StringComparison.CurrentCultureIgnoreCase) &&
string.Equals(item.GetType().Name, animal,
StringComparison.CurrentCultureIgnoreCase));
});

if (thisAnimal != null)
throw new ArgumentException("Duplicate name for same animalType
is not allowed");
}

//Tony

 

[Arne Vajhøj]

This Add method below is called when I want to add an animal to the animal
collection that is located in the AnimalManager class.In the ValidateData
method that is called from the Add method is used for checking some business
rules.
In this method ValidateData I use the GetType to get the dynamic type but
somebody of you mentioned that it was not appropriate to do in the way that
I now do but instead should use a property.

The need to check on the type is a symptom that the OO design
has a problem.

This animal parameter that is passed to this Add method is comming from a
selection in a ListBox in the GUI class.
So I just want to know how the implementation of this property should look
like.

animalManager.Add(category, animal, txtName.Text,
double.Parse(txtAge.Text.Trim()), gender, housing, eater, false,
foodMenuList);

public void Add(CategoryType category, string animal, string name, double
age, GenderType gender,HousingType housing,
EaterType eater, bool aggressive, List<string>
foodMenuList)
{
//Check some business rules
ValidateData(name, animal);

//Generate the ID that is unique each animal in the collection
string ID = GenerateID();

switch (category)
{
case CategoryType.Mammal:
MammalSpices mammalSpecies =
(MammalSpices)Enum.Parse(typeof(MammalSpices), animal);
animals.Add( (Animal)
MammalFactory.CreateMammal(mammalSpecies, name,ID, age, gender, housing,
eater, false, foodMenuList));
break;

case CategoryType.Bird:
BirdSpices
birdSpecies=(BirdSpices)Enum.Parse(typeof(BirdSpices),animal);
animals.Add((Animal)BirdFactory.CreateBird(birdSpecies, name,
ID, age,gender, housing, eater, false,
foodMenuList));
break;

case CategoryType.Insect:
InsectSpecies insectSpecies =
(InsectSpecies)Enum.Parse(typeof(InsectSpecies),animal);
animals.Add((Animal)InsectSpecies.CreateBird(insectSpecies,
name, ID, age, gender, housing, eater, false,
foodMenuList));
break;

case CategoryType.Marine :
MarineSpices marineSpecies =
(MarineSpices)Enum.Parse(typeof(MarineSpices), animal);
animals.Add((Animal)MarineSpices.CreateBird(marineSpecies,
name, ID, age, gender, housing, eater, false,
foodMenuList));
break;

case CategoryType.Reptile :
ReptileSpecies reptileSpecies =
(ReptileSpecies)Enum.Parse(typeof(ReptileSpecies), animal);
animals.Add((Animal)ReptileSpecies.CreateBird(reptileSpecies,
name, ID, age, gender, housing, eater, false,
foodMenuList));
break;
}
}

//Check the business rules that say there must not be two names for the same
animalType
private void ValidateData(string name, string animal)
{
Animal thisAnimal = (Animal)animals.Find(delegate(Animal item)
{
return (string.Equals(item.Name, name,
StringComparison.CurrentCultureIgnoreCase)&&
string.Equals(item.GetType().Name, animal,
StringComparison.CurrentCultureIgnoreCase));
});

if (thisAnimal != null)
throw new ArgumentException("Duplicate name for same animalType
is not allowed");
}

I would structure it completely differently.

You have an abstract Animal base class and a number of
concrete sub classes.

You have an AnimalManager class - if purely used for
storing them, then a List<Animal> could do, but if you need
additional functionality, then AnimalManager is fine.

The AnimalManager Add should just take an Animal as argument.

The controller code get the Animal created and Add it to
AnimalManager.

The format validation (that age is actually a number and not "ABC")
is done by validation rules in the UI.

The logical consistency / semantic data validation is done
in either the concrete sub class or in the factory for that
concrete sub class.

When you validate there you don't need to check on type
because you know what you got.

Arne

 

[Tony Johansson]

The need to check on the type is a symptom that the OO design
has a problem.

Yes I will try to remove this check on type.

I would structure it completely differently.

You have an abstract Animal base class and a number of
concrete sub classes.
Yes

You have an AnimalManager class - if purely used for
storing them, then a List<Animal> could do, but if you need
additional functionality, then AnimalManager is fine.
Yes

The AnimalManager Add should just take an Animal as argument.

But how do you mean here you say "The AnimalManager Add should just take an
Animal as argument"
but the Animal class is abstract so it can't be instansiated.
So how do you mean that I can implement in the way that you say
Can you explain that ?

The controller code get the Animal created and Add it to
AnimalManager.

I don't fully undersyand how I can implamant this.

The format validation (that age is actually a number and not "ABC")
is done by validation rules in the UI.

What do you mean here ?

The logical consistency / semantic data validation is done
in either the concrete sub class or in the factory for that
concrete sub class.

When you validate there you don't need to check on type
because you know what you got.

Can you give a simple example ?

 

[Arne Vajhøj]

Yes I will try to remove this check on type.

But how do you mean here you say "The AnimalManager Add should just take an
Animal as argument"
but the Animal class is abstract so it can't be instansiated.
So how do you mean that I can implement in the way that you say
Can you explain that ?

Animal a = new Dog(foo, bar, bla, bla);
animalmgr.Add(a);

I don't fully undersyand how I can implamant this.

See above.

What do you mean here ?


Can you give a simple example ?

Let me see if I find the time to create a reasonable
complete example.

Arne

 

[Tony Johansson]

Animal a = new Dog(foo, bar, bla, bla);
animalmgr.Add(a);


See above.


Let me see if I find the time to create a reasonable
complete example.

Arne

I have now created an abstract ValidationData in the abstract base class
Animal and then implemented this method in the derived classes. This works
good but the main point is that I want to check if there already exist a
name for the same animal type.
When this ValidationData is called for the dynamic type I don't have a
reference to the controller class AnimalManager.
In the controller class is the collection of animals stored. So one way to
solve this is to pass along a reference to this animal collection when this
ValidationData is called polymorfiskt or pass along this which is the
controller class AnimalManager.
What would you prefer ? There might exist other way to solve this then these
two I mantioned.

//Tony

 

[Arne Vajhøj]

I have now created an abstract ValidationData in the abstract base class
Animal and then implemented this method in the derived classes. This works
good but the main point is that I want to check if there already exist a
name for the same animal type.
When this ValidationData is called for the dynamic type I don't have a
reference to the controller class AnimalManager.
In the controller class is the collection of animals stored. So one way to
solve this is to pass along a reference to this animal collection when this
ValidationData is called polymorfiskt or pass along this which is the
controller class AnimalManager.
What would you prefer ? There might exist other way to solve this then these
two I mantioned.

I think the duplication check should be done in AnimalManager and
the validation code described should check the internal consistency
of the Animal.

It is really two different things.

Arne

 

[Tony Johansson]

Hello!

Here is my structure of code now roughly.
1 In the global section of GUI class I create the controller class
AnimalManager.
2. I make my selection in the GUI where I for example select to have
category mammal and animal type Cat and the Gender female and the food cats
like ans such things.
3.I click on the event handler AddAnimal that looks like this
private void BtnAddAnimal_Click(object sender, EventArgs e)
{
if (!ValidateInput())
return;

string name = txtName.Text.Trim();
CategoryType category =
(CategoryType)Enum.Parse(typeof(CategoryType), lstCategory.Text);
string animal = lstAnimalType.Text;
EaterType eater = (EaterType)Enum.Parse(typeof(EaterType),
lstEaterType.Text);
GenderType gender = (GenderType)Enum.Parse(typeof(GenderType),
lstGender.Text);
HousingType housing = (HousingType)Enum.Parse(typeof(HousingType),
cboHousing.Text);
List<FoodItem> foodMenuList = new List<FoodItem>();

foreach (FoodItem itemChecked in checkedListBox.CheckedItems)
foodMenuList.Add(itemChecked);

try
{
animalManager.Add(category, animal, txtName.Text,
double.Parse(txtAge.Text.Trim()), gender, housing, eater,
false, foodMenuList);
}
catch (ArgumentException ex)
{
MessageBox.Show(ex.Message);
}
UpdateGUI();
}

4 Here is the Add method in the controller class AnimalManager
public void Add(CategoryType category, string animal, string name, double
age, GenderType gender,
HousingType housing, EaterType eater, bool aggressive,
List<FoodItem> foodMenuList)
{
//Check some business rules
ValidateData(name, animal);

//Generate the ID that is unique each animal in the collection
string ID = GenerateID();

switch (category)
{
case CategoryType.Mammal:
MammalSpices mammalSpecies =
(MammalSpices)Enum.Parse(typeof(MammalSpices), animal);
animals.Add(
(Animal)MammalFactory.CreateMammal(mammalSpecies, name, ID, age, gender,
housing, eater,
false, foodMenuList));
break;

case CategoryType.Bird:
BirdSpices birdSpecies =
(BirdSpices)Enum.Parse(typeof(BirdSpices), animal);
animals.Add((Animal)BirdFactory.CreateBird(birdSpecies, name,
ID, age, gender, housing, eater, false,
foodMenuList));
break;

case CategoryType.Insect:
InsectSpecies insectSpecies =
(InsectSpecies)Enum.Parse(typeof(InsectSpecies), animal);
animals.Add((Animal)InsectFactory.CreateInsect(insectSpecies,
name, ID, age, gender, housing, eater, false,
foodMenuList));
break;

case CategoryType.Marine :
MarineSpices marineSpecies =
(MarineSpices)Enum.Parse(typeof(MarineSpices), animal);
animals.Add((Animal)MarineFactory.CreateMarine(marineSpecies,
name, ID, age, gender, housing, eater, false,
foodMenuList));
break;

case CategoryType.Reptile :
ReptileSpecies reptileSpecies =
(ReptileSpecies)Enum.Parse(typeof(ReptileSpecies), animal);
animals.Add((Animal)ReptileFactory.CreateReptile(reptileSpecies,
name, ID, age, gender, housing, eater, false,
foodMenuList));
break;
}
}

5. Here I just show you the factory class for category MammalFactory that
use the static method CreateMammal
All factory classes like Reptilefactory,Birdfactory and so on is built in
the same way
static class MammalFactory
{
public static IAnimal CreateMammal(MammalSpices mammalSpecies,
string name, string id, double age,
GenderType gender, HousingType housing,
EaterType eater, bool aggressive,
List<FoodItem> foodMenyList)
{
IAnimal objbase = null;
switch (mammalSpecies)
{
case MammalSpices.Dog:
objbase = new Dog(name, id, age, gender, housing, eater,
aggressive, foodMenyList);
break;

case MammalSpices.Cat:
objbase = new Cat(name, id, age, gender, housing, eater,
aggressive, foodMenyList);
break;

case MammalSpices.Horse:
objbase = new Cat(name, id, age, gender, housing, eater,
aggressive, foodMenyList);
break;

default:
throw new ArgumentOutOfRangeException("No mammal animal
passed to be created");
}
return objbase;
}
}

You wrote
In my structure of code it's not possible to call the add method in the
controller class AnimalManager and pass the
Animal object in the way that you gave an example on Animal a = new Dog(foo,
bar, bla, bla); animalmgr.Add(a);

So in what way do you mean that I could use your example in the present code
structure to
call the add method in the controller class AnimalManger and pass the animal
object itself.

//Tony