patterncsharpMinor
Character inventory in C#
Viewed 0 times
inventorycharacterstackoverflow
Problem
Here are the relevant parts of a project I'm working on that's working perfectly. I was wondering what I should change to improve it (to respect SOLID principle for example), especially regarding my add/remove loot functions which, even though they work, don't really feel right.
```
public class Hero
{
private int gold { get; set; }
public event GoldHandler GoldChanged;
public delegate void GoldHandler(Hero m, int goldChange);
public BackPack backPack { get; set; }
public event backPackHandler backPackChanged;
public delegate void backPackHandler(Hero m, Item item, bool add);
public WeaponHolder weaponHolder { get; set; }
public event weaponHolderHandler weaponHolderChanged;
public delegate void weaponHolderHandler(Hero m, Weapon weapon, bool add);
private Hero()
{
}
public Hero(string name)
{
this.gold = 0;
backPack = new BackPack();
weaponHolder = new WeaponHolder();
}
public void addLoot(Loot loot)
{
if (loot is Weapon)
{
this.addWeapon((Weapon)loot);
return;
}
if (loot is Item)
{
this.addBackPackItem((Item)loot);
return;
}
if (loot is Gold)
{
this.addGold(((Gold)loot).getGoldAmount);
}
}
public void removeLoot(Loot loot)
{
if (loot is Weapon)
{
this.removeWeapon((Weapon)loot);
return;
}
if (loot is Item)
{
this.removeBackPackItem((Item)loot);
return;
}
if (loot is Gold)
{
this.removeGold(((Gold)loot).getGoldAmount);
}
}
public int getGold()
{
return this.gold;
}
public void addGold(int gold)
{
this.gold += gold;
GoldHasChanged(gold);
}
public void removeGold(int gold)
{
if ((this.gold - gold) >= 0)
{
this
```
public class Hero
{
private int gold { get; set; }
public event GoldHandler GoldChanged;
public delegate void GoldHandler(Hero m, int goldChange);
public BackPack backPack { get; set; }
public event backPackHandler backPackChanged;
public delegate void backPackHandler(Hero m, Item item, bool add);
public WeaponHolder weaponHolder { get; set; }
public event weaponHolderHandler weaponHolderChanged;
public delegate void weaponHolderHandler(Hero m, Weapon weapon, bool add);
private Hero()
{
}
public Hero(string name)
{
this.gold = 0;
backPack = new BackPack();
weaponHolder = new WeaponHolder();
}
public void addLoot(Loot loot)
{
if (loot is Weapon)
{
this.addWeapon((Weapon)loot);
return;
}
if (loot is Item)
{
this.addBackPackItem((Item)loot);
return;
}
if (loot is Gold)
{
this.addGold(((Gold)loot).getGoldAmount);
}
}
public void removeLoot(Loot loot)
{
if (loot is Weapon)
{
this.removeWeapon((Weapon)loot);
return;
}
if (loot is Item)
{
this.removeBackPackItem((Item)loot);
return;
}
if (loot is Gold)
{
this.removeGold(((Gold)loot).getGoldAmount);
}
}
public int getGold()
{
return this.gold;
}
public void addGold(int gold)
{
this.gold += gold;
GoldHasChanged(gold);
}
public void removeGold(int gold)
{
if ((this.gold - gold) >= 0)
{
this
Solution
WeaponTypes
The
Hero
Your hero uses a backpack and a weapon-holder classes but instead using them and adding items there you perform the actions on the hero where you implement add-weapon or add-loot.
Loot
This base class should provide a protected constructor that requries you to specify a name. This way you won't forget it. The
If you make the
Weapon
You don't need a private default constructor that you don't use anyway. As soon as you define a custom one the default one is no longer automaitcally created.
WeaponHolder/BackPack
Instead of having a
In fact you actually need one of them and rename it to something like
Consummable/Item/Food
Food is consumable so food should have the members that the consummable currently has. This class is redundant... but you don't use it anywhere :-|
Names
In C# we use PascalCase for all public members.
public enum WeaponTypes
{
Sword,
Spear,
Mace,
Dagger,
Sabre,
WarHammer,
Axe,
Baton,
TwoEdgedSword,
None
}The
None value should be first as the first value is always the default value if don't set any other. You could provide another field that you call Default and set it to any other field but Sword by default is counter intuitive to me.Hero
Your hero uses a backpack and a weapon-holder classes but instead using them and adding items there you perform the actions on the hero where you implement add-weapon or add-loot.
Loot
This base class should provide a protected constructor that requries you to specify a name. This way you won't forget it. The
Name setter should be in this case private.If you make the
Name property public you don't need the getName method which is redundant anyway. We have access modifiers so that we don't have to write such methods. (the same situation with weapon).Weapon
You don't need a private default constructor that you don't use anyway. As soon as you define a custom one the default one is no longer automaitcally created.
WeaponHolder/BackPack
Instead of having a
getWeapons method there, your WeaponHolder could implement the IEnumerable interface and return the values instead of revealing the internal list.In fact you actually need one of them and rename it to something like
ItemHolder and make it generic. This could then hold either weapons or loot as they are nearly identical. You just create two different instances of it new ItemHolder() and new ItemHolder()Consummable/Item/Food
Food is consumable so food should have the members that the consummable currently has. This class is redundant... but you don't use it anywhere :-|
Names
In C# we use PascalCase for all public members.
Code Snippets
public enum WeaponTypes
{
Sword,
Spear,
Mace,
Dagger,
Sabre,
WarHammer,
Axe,
Baton,
TwoEdgedSword,
None
}Context
StackExchange Code Review Q#138422, answer score: 3
Revisions (0)
No revisions yet.