Will using one class per inventory item create issue on the long term?

Question

I finally managed to plug my menu to my inventory system, display current items, use them etc. But the way I did it... makes me feel dirty.

To cut short, my items are described this way:

using System;

public abstract class Item {
    public abstract string Name { get; }
    public abstract string Description { get; }
    public virtual bool IsUsable { get { return false; } }
    public virtual bool IsEquippable { get { return false; } }
    public virtual bool NeedTarget { get { return false; } }
    public virtual void OnUse() { }
}

public class Potion : Item {
    public override string Name { get { return "Potion"; } }
    public override string Description { get { return "Restores 100HP to one character"; } }
    public override bool IsUsable { get { return true; } }
    public override bool NeedTarget { get { return true; } }

    public void OnUse(PlayerCharacter target) {
        Effects.HealTarget(target, 100);
    }

    public override void OnUse() {
        OnUse(GameData.getFirstPc());
    }
}

public class Poison : Item {
    public override string Name { get { return "Poison"; } }
    public override string Description { get { return "Deals 100HP damage to one character"; } }
    public override bool IsUsable { get { return true; } }
    public override bool NeedTarget { get { return true; } }

    public void OnUse(PlayerCharacter target) {
        Effects.DamageTarget(target, 50);
    }

    public override void OnUse() {
        OnUse(GameData.getFirstPc());
    }
}

My Inventory is declared like this:

public class Inventory {
    private static Dictionary<string, Item> ItemDB = new Dictionary<string, Item>() {
        { "00001", new Potion()},
        { "00002", new Poison()},
        { "00003", new HiPotion()},
        { "00004", new FangOfDestroyer()},
        { "00005", new Elixir()}
    };

    public static Dictionary<string, int> CarriedInventory = new Dictionary<string, int>();
}

When I want to add items to CarriedInventory I basically call CarriedInventory[itemID] = newAmount

Everything only uses the ID, and when I want to get an item's name I call ItemDB[itemID].Name, when I want to use an item I call ItemDB[itemID].OnUse() , etc

My reasons for doing this were:

• I can be as specific as I want on item uses as I want, I can have unique item effects without having to worry about extending it to all other items
• I can use the effects in the Effects class for general effects (heal, damage, etc)
• if later on I realize I need a new property for an item, I can just add a virtual property as default to the Item class (e.g. I realize I want an item to not be droppable, I add a public virtual bool IsDroppable with a value of true, and non-droppable items can override this
• I avoid inheritance hell

And... it works, it works, but it feels wrong.

Is it wrong? How wrong?

Is there a better way to do it?

Answer 1

There is nothing wrong with how you decided to design the inventory system. Keep in mind though that different decisions are best for different games.

The only problem I can think with your approach, is that this system does not scale that well. What I mean by that is, if during the development of your game you create 100+ items, it will be very difficult to maintain all of them. However if your game has just a handful of different items, you will not run into this problem.

Here's a few improvements I can think, that might work for some games, but not for others:

• To make your items a little bit more versatile, one thing you can do is, instead of making one class for each item, make one class for each item type. To use your example, you can use one class called Potion, and use that for all potions, regardless of how much they heal. Inside the Constructor of that class, let it take an argument of how much health it heals. So to create a normal potion you would need to do (for example) new Potion(50) to make a potion that heals 50 HP, but to make a HiPotion you could use new Potion(100), which heals 100 HP.
• Similar to the note above, instead of using raw numbers (like new Potion(50) above) you can use a variable to create "Variations" of an item. In the potion example, you could do something like new Potion(1), to create a "level 1" potion, which heals 50HP, and new Potion(2) to create a "level 2" potion, which is now called HiPotion instead, and heals more HP.

These improvements can create a range of items without duplicating as much code. Think about it this way, Potion and HiPotion both do the same thing (I assume on your game at least), just with different values, in that case do they really need to duplicate the code of their behaviour? This logic can also be applied for weapons and armor.