6
\$\begingroup\$

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.gold -= gold;
            GoldHasChanged(-gold);
        }

    }
    public void emptyGold()
    {
        int tempoGold = this.gold;
        this.gold = 0;
        GoldHasChanged(-tempoGold);
    }
    public void GoldHasChanged(int gold)
    {
        GoldHandler handler = GoldChanged;
        if (handler != null)
        {
            handler((Hero)this, gold);
        }
    }
    private void addWeapon(Weapon weapon)
    {
        this.weaponHolder.Add(weapon);
        weaponHolderHasChanged(weapon, true);
    }
    private void removeWeapon(Weapon weapon)
    {
        this.weaponHolder.Remove(weapon);
        weaponHolderHasChanged(weapon, false);
    }
    public void weaponHolderHasChanged(Weapon weapon, bool add)
    {
        weaponHolderHandler handler = weaponHolderChanged;
        if (handler != null)
        {
            handler((Hero)this, weapon, add);
        }
    }

    private void addBackPackItem(Item item)
    {
        this.backPack.Add(item);
        backPackItemHasChanged(item, true);
    }
    private bool removeBackPackItem(Item item)
    {
        if (this.backPack.Remove(item))
        {
            backPackItemHasChanged(item, false);
            return true;
        }
        else
            return false;
    }
    public void backPackItemHasChanged(Item item, bool add)
    {
        backPackHandler handler = backPackChanged;
        if (handler != null)
        {
            handler((Hero)this, item, add);
        }
    }

    public void useItem(Item item)
    {
            item.use(this);
            backPackItemHasChanged(item, false);
    }

}

public abstract class Loot
{

}
public class Gold : Loot
{
    int goldAmount;
    private Gold()
    {

    }
    public Gold(int amount)
    {
        this.goldAmount = amount;
    }
    public int getGoldAmount
    {
        get { return this.goldAmount; }
    }
}
public abstract class Item : Loot
{
    protected string name{get;set;}

    public string getName
    {
        get { return name; }
    }
    public abstract void use(Hero hero);
}

public class Consummable : Item
{

    int healingPower{get;set;}
    int chargesLeft { get; set; }

    private Consummable()
    {

    }
    public Consummable(string name, int healingPower, int charges)
    {
        this.healingPower = healingPower;
        this.name = name;
        this.chargesLeft = charges;
    }
    public override void use(Hero hero)
    {
        if (chargesLeft >= 1)
        {
            this.chargesLeft--;
        }
    }

}

public class Food : Item
{
    int chargesLeft { get; set; }
    private Food()
    {

    }
    public Food(string name, int charges)
    {
        this.name = name;
        this.chargesLeft = charges;
    }

    public override void use(Hero hero)
    {
        if (chargesLeft >= 1)
        { 
            this.chargesLeft--;
        }
    }

}



public class Weapon : Loot
{
    private string name{get;set;}
    private WeaponTypes weaponType { get; set; }

    private Weapon()
    {

    }
    public Weapon(string name, WeaponTypes weaponType){
        this.name = name;
        this.weaponType = weaponType;
    }

    public WeaponTypes getWeaponType
    {
        get { return weaponType; }
    }
}
public enum WeaponTypes
{
    Sword,
    Spear,
    Mace,
    Dagger,
    Sabre,
    WarHammer,
    Axe,
    Baton,
    TwoEdgedSword,
    None
}


public class WeaponHolder
{
    List<Weapon> weapons;

    public WeaponHolder()
    {
        weapons = new List<Weapon>();
    }

    public void Add(Weapon weapon)
    {
            this.weapons.Add(weapon);
    }
    public void Remove(Weapon weapon)
    {
        this.weapons.Remove(weapon);
    }

    public bool isEmpty()
    {
        if (this.weapons.Count == 0)
            return true;
        else
            return false;
    }

    public List<Weapon> getWeapons
    {
        get { return weapons;}
    }

}


 public class BackPack
{
    public List<Item> items;

    public BackPack()
    {
        this.items = new List<Item>();
    }

    public void Add(Item backPackItem)
    {
            this.items.Add(backPackItem);
    }

    public bool Remove(Item backPackItem)
    {
        return this.items.Remove(backPackItem);
    }

    public List<Item> getItems
    {
        get { return items; }
    }

}
\$\endgroup\$
  • \$\begingroup\$ From what it looks like, you have 3 model objects (model as in MVC), that is: an observable object that informs observers about a change in its value. The difference between the 3 is just the type of the value (Weapon, Item or Gold). I suspect that you could write a generic Model<T> class (possibly used as base class) and save a lot of duplicated code that you have right now. \$\endgroup\$ – I'll add comments tomorrow Aug 11 '16 at 13:25
3
\$\begingroup\$

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<Weapon> 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<TItem> 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<Weapon>() and new ItemHolder<Loot>()

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your advices, I'll look into that. Just regarding my " private default constructor that I don't use anyway", in fact it's a "trick" required to make my class works with EntityFrameWork, it requires an empty constructor in order to work \$\endgroup\$ – Belterius Aug 11 '16 at 9:45
  • \$\begingroup\$ @Belterius oh, I couldn't have known that, then it's ok ;-) \$\endgroup\$ – t3chb0t Aug 11 '16 at 9:46
  • 1
    \$\begingroup\$ @Belterius less code it usually better but sometimes it's inevitable if it's something like a framework... in your case I find it's perfect. \$\endgroup\$ – t3chb0t Aug 11 '16 at 9:53
  • 1
    \$\begingroup\$ @t3chb0t Regarding enum defaults, it would be more precise to say that the enum corresponding to zero is set as default (even if you have none!) \$\endgroup\$ – 404 Aug 11 '16 at 17:27
  • 1
    \$\begingroup\$ @eurotrash I didn't know that :-o \$\endgroup\$ – t3chb0t Aug 11 '16 at 17:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.