HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Character inventory in C#

Submitted by: @import:stackexchange-codereview··
0
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

Solution

WeaponTypes

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.