16
\$\begingroup\$

what do you think about this code? It's upgraded version of: Classes representing items in an RPG game

using System;
using System.Collections.Generic;

public class Item
{
    public string Name { get; protected set; }
    public int Cost { get; protected set; }

    public override string ToString()
    {
        return "Name: " + Name + "\nCost: " + Cost;
    }

    public Item(string name, int cost)
    {
        Name = name;
        Cost = cost;
    }
}

public abstract class SpecialItem : Item
{
    public virtual int GetSpecialProperty { get; }
    public override string ToString() { return base.ToString() + "\nDamages/ArmorPts/HungryRestorePts: " + GetSpecialProperty; }
    public SpecialItem(string name, int cost) : base(name, cost) { }
}

public abstract class ElementalsItem : SpecialItem
{
    public int Fire  { get; protected set; }
    public int Water { get; protected set; }
    public int Earth { get; protected set; }
    public int Wind  { get; protected set; }
    public override string ToString()
    {
        return base.ToString()
            + "\nFire Damages/Resistance: " + Fire
            + "\nWater Damages/Resistance: " + Water
            + "\nEarth Damages/Resistance: " + Earth
            + "\nWind Damages/Resistance: " + Wind;
    }
    protected ElementalsItem(string name, 
                             int cost,
                             int fire, 
                             int water,
                             int earth, 
                             int wind)
                             : base(name, cost)
    {
        Fire  = fire;
        Water = water;
        Earth = earth;
        Wind  = wind;
    }
}

public class Weapon : ElementalsItem
{
    public int Damages { get; private set; }
    public override int GetSpecialProperty { get { return Damages; }}
    public override string ToString() { return base.ToString(); }

    public Weapon(string name,
                  int cost,
                  int damages,
                  int fireDamages  = 0,
                  int waterDamages = 0, 
                  int earthDamages = 0,
                  int windDamages  = 0)
                  : base(name, cost, fireDamages, waterDamages, earthDamages, windDamages)

    {
        Damages = damages;
    }
}

public class Armor : ElementalsItem
{
    public int ArmorPts { get; private set; }

    public override int GetSpecialProperty { get { return ArmorPts; }}
    public override string ToString() { return base.ToString(); }

    public Armor(string name,
                 int cost,
                 int armorPts,
                 int fireResistance  = 0,
                 int waterResistance = 0,
                 int earthResistance = 0,
                 int windResistance  = 0) 
                 : base(name, cost, fireResistance, waterResistance, earthResistance, windResistance)
    {
        ArmorPts = armorPts;
    }
}

public class Food : SpecialItem
{
    public int HungryRestorePts { get; private set; }
    public override int GetSpecialProperty { get { return HungryRestorePts; }}
    public override string ToString() { return base.ToString(); }

    public Food(string name, int cost, int hungryRestorePts) : base(name, cost)
    {
        HungryRestorePts = hungryRestorePts;
    }
}

class Equipment
{
    public static void Test()
    {
        List<Item> items = new List<Item>();
        items.AddRange(new Item[] { new Item("Stone", 1),
                                    new Armor("Steel Helmet", 80, 10),
                                    new Weapon("Great Sword", 120, 80),
                                    new Armor("Storm Cuirass", 1000, 120, 0, 0, 0, 60),
                                    new Weapon("Fire Spear", 1400, 60, 0, 80, 0, 0),
                                    new Food("Apple", 5, 10) });
        foreach(Item i in items)
        {
            Console.WriteLine(i.ToString());
            Console.WriteLine();
        }
    }
}
\$\endgroup\$
2
  • 4
    \$\begingroup\$ Honestly, I find the whole design really over-thought and cumbersome without adding a lot of value. There is value in having a shallower inheritance tree, favoring composition over inheritance. But the existing answers cover pretty well improvements on the existing code w/o a complete rewrite. \$\endgroup\$
    – Paul
    Commented Dec 26, 2016 at 14:50
  • \$\begingroup\$ @Paul can you show me your way to write this? I will be grateful. :) \$\endgroup\$
    – TKK
    Commented Dec 26, 2016 at 18:17

5 Answers 5

23
\$\begingroup\$

Edge cases are where such class hierarchies often fall short.

  1. An offhand short sword that adds to defense?
  2. A shield that does damage when you bash with it?
  3. A magic potion that not only makes you less hungry but also adds armor when you drink it?

How will those fit into your current hierarchy? One way to avoid those problems is to use aggregation instead of inheritance and treat each game object as a collection of arbitrary properties, that can be added to object or removed from it. You might want to look at how, for example, things are done in Unity, to get a better understanding of how things are actually designed in modern engines.

To be clear, there is nothing wrong with having game object hierarchies in small projects. It is the easiest approach and sometimes it is enough, especially if all you want is to write some code. I just wanted to give you some perspective.

EDIT Since this question is getting a lot of attention I though I might as well update my answer with a simplified example.

First, you have to create some "components". For the sake of this example, you can think of components as properties that define the item and make it different and special. What is a fire sword? It is an item that:

  • people in the village call "Lightbringer"
  • requires some "sword" skill to use
  • looks like a big burning knife
  • can do slicing damage to things
  • but does fire damage too
  • it weights 8 kg
  • it costs 1000g and can be bought or sold
  • etc...

Those are the aspects of a fire sword that make it what it is. And every one of those aspects can be defined as separate component.

abstract class ItemComponent
{
    //What goes into base class entirely depends 
    //on how your game engine is implemented.
    //I left it out because it is irrelevant 
    //in context of this example
}

class Valuable : ItemComponent
{
    public int Cost { get; set; }

    public override string ToString()
    {
        return "It is a valuable item, that can be bought for " + Cost + "g.";
    }
}

class PhysicalDamage : ItemComponent
{
    public int Slashing { get; set; }
    public int Bashing { get; set; }
    public int Piercing { get; set; }

    public override string ToString()
    {
        return String.Format("It does {0}/{1}/{2} physical damage.", Slashing, Bashing, Piercing);
    }
}

class ElementalDamage : ItemComponent
{
    public int Fire { get; set; }
    public int Water { get; set; }
    public int Earth { get; set; }
    public int Wind { get; set; }

    public override string ToString()
    {
        return String.Format("It does {0}/{1}/{2}/{3} elemental damage.", Fire, Water, Earth, Wind);
    }
}

//etc...

And this is a typical game object that theoretically can represent anything from a stone on the ground to a bullet. It's just a collection of arbitrary components.

class Item
{
    public Item(string name)
    {
        Name = name;
    }
    //Name can be a component too! 
    public string Name { get; private set; }

    public Item AddComponent(ItemComponent component)
    {
        _components.Add(component);
        return this;
    }

    public T GetComponent<T>()
    {
        return (T)_components.OfType<T>().FristOrDefault();
    }

    public override string ToString()
    {
        var sb = new StringBuilder();
        sb.AppendLine(Name);
        _components.ForEach(c => sb.AppendLine(c.ToString()));
        return sb.ToString();
    }

    private List<ItemComponent> _components = new List<ItemComponent>();
}

Then to create a fire sword all you need to do is create an item with all the necessary aspects:

var fireSword = new Item("Lightbringer")
                   .AddComponent(new Valuable { Cost = 1000 })
                   .AddComponent(new PhysicalDamage { Slashing = 10 })
                   .AddComponent(new ElementalDamage { Fire = 10 })
//Maybe it also makes you weak to water attacks? 
//No problem, just define and add new component
                   .AddComponent(new ElementalResistance { Water = -10 };
Console.WriteLine(fireSword.ToString())

This allows you to do all sorts of neat stuff later on. For example, to calculate total resistances of a player, you can call GetComponent<ElementalResistance>() for every equipped item and sum the result.

\$\endgroup\$
3
  • \$\begingroup\$ Really good points there, things that should definitely be taken in account when making a medium size or bigger game, in which case composition over inheritance is the way to go usually. \$\endgroup\$
    – Denis
    Commented Dec 26, 2016 at 16:02
  • \$\begingroup\$ Thanks for good tips, they will be useful in bigger projects. :) \$\endgroup\$
    – TKK
    Commented Dec 26, 2016 at 18:07
  • \$\begingroup\$ I was thinking about a similar way to do this, but I wasn't sure about this. Now I see a simplicity and shapeliness of this way, and I think that is the best solution. :) \$\endgroup\$
    – TKK
    Commented Dec 28, 2016 at 10:38
15
\$\begingroup\$
  1. Abstract cant be instantiated so having a public constructor for them is pretty much useless as they are being created through derived classes and it makes more sense to have a protected constructor instead of a public one

    public abstract class SpecialItem : Item
    {
         public virtual int GetSpecialProperty { get; }
         public override string ToString() { return base.ToString() +"\nDamages/ArmorPts/HungryRestorePts: " +  GetSpecialProperty; }
         public SpecialItem(string name, int cost) : base(name, cost) { }
    }
    

    Can become:

    public abstract class SpecialItem : Item
    {
        public virtual int GetSpecialProperty { get; }
        public override string ToString() { return base.ToString() + "\nDamages/ArmorPts/HungryRestorePts: " + GetSpecialProperty; }
        protected SpecialItem(string name, int cost) : base(name, cost) { }
    }
    
  2. Virtual function without any functionality should rather be marked as abstract.

    public virtual int GetSpecialProperty { get; }
    

    Can become:

    public abstract int GetSpecialProperty { get; }
    
  3. Enviornement.NewLine instead of \n

    public override string ToString()
    {
        return base.ToString()
            + "\nFire Damages/Resistance: " + Fire
            + "\nWater Damages/Resistance: " + Water
            + "\nEarth Damages/Resistance: " + Earth
            + "\nWind Damages/Resistance: " + Wind;
    }
    

    With C#6' interpolated strings can become:

        return $"{base.ToString()} {Environment.NewLine} " +
               $"Fire Damages/Resistance:  {Fire} {Environment.NewLine} " +
               $"Water Damages/Resistance:  {Water} {Environment.NewLine} " +
               $"Earth Damages/Resistance:  {Earth} {Environment.NewLine} " +
               $"Wind Damages/Resistance:  {Wind} {Environment.NewLine}";
    
  4. Redundant overriding

    You are overriding the ToString() in Armor, Weapon, Food but you are doing nothing but calling the base method, so you can just remove it.

    public override string ToString() { return base.ToString(); }
    
  5. Readonly properties

    public int HungryRestorePts { get; private set; }
    public int ArmorPts { get; private set; }
    public int HungryRestorePts { get; private set; }
    

    Are rather readonly properties than properties with private sets, you are giving them value only in the constructor anyway with C#6 they can become :

    public int HungryRestorePts { get; }
    public int ArmorPts { get; }
    public int HungryRestorePts { get; }
    
  6. Expression bodied properties

    public override int GetSpecialProperty { get { return Damages; } }
    

    If you are using C# 6 it can be shorten to:

    public override int GetSpecialProperty => Damages;
    
  7. Long constructors

    Your Armor & Weapon class have too much parameters making the initialization ugly also they seem like a immutable objects, you might want to apply the builder design pattern, you can check my answer on t3chb0t's question here, which if you configure properly can make your intialization a lot prettier + your objects will be immutable.

Update

Implementing the builder pattern in your case might be difficult since you have inheritance going on, especially if you want to have a generic item builder. I will show you what I came up with tho I believe it can be made in a better way, for which I have an alternative solution.

Builder pattern

Pros

  1. Simplifies the creation of the item classes.
  2. Provides full immutability (all of your properties can become get only).
  3. Item properties can be set in any order.
  4. Allows different type of object to be created using the same builder.

Cons

  1. It will be hard to create good relation or hierarchy between the builders in order to avoid duplicate code (you will see what I mean in a second).

Having that in mind I present you my Builder pattern

public class BaseItemBuilder
{
    private string _name;
    private int _cost;

    public BaseItemBuilder WithNameAndCost(string name, int cost)
    {
        _name = name;
        _cost = cost;
        return this;
    }

    public Item Build()
    {
        return new Item(_name, _cost);
    }
}

Nothing fancy here. Simple builder to create objects of type Item.

Example usage:

BaseItemBuilder baseItemBuilder = new BaseItemBuilder();
Item itemStone = baseItemBuilder
    .WithNameAndCost("Stone", 1)
    .Build();
Item itemPot = baseItemBuilder
    .WithNameAndCost("Red Pot", 2)
    .Build();

This looks pretty neat, but let's move to the next builder which can help you to create ElementalItems:

public class ElementalItemBuilder
{
    private string _name;
    private int _cost;
    private int _fire;
    private int _water;
    private int _earth;
    private int _wind;
    private int _points;

    public ElementalItemBuilder WithNameAndCost(string name, int cost)
    {
        _name = name;
        _cost = cost;
        return this;
    }

    public ElementalItemBuilder WithFire(int fire)
    {
        _fire = fire;
        return this;
    }

    public ElementalItemBuilder WithWater(int water)
    {
        _water = water;
        return this;
    }

    public ElementalItemBuilder WithEarth(int earth)
    {
        _earth = earth;
        return this;
    }

    public ElementalItemBuilder WithWind(int wind)
    {
        _wind = wind;
        return this;
    }

    public ElementalItemBuilder WithPoints(int points)
    {
        _points = points;
        return this;
    }

    public TItem Build<TItem>() where TItem : ElementalsItem
    {
        return (TItem)Activator.CreateInstance(typeof(TItem), _name, _cost, _points, _fire, _water, _earth, _wind);
    }
}

Example usage:

ElementalItemBuilder equipementItemBuilder = new ElementalItemBuilder();
Armor helmet = equipementItemBuilder
    .WithNameAndCost("Steel Helmet", 80)
    .WithPoints(30)
    .WithEarth(1)
    .WithFire(2)
    .Build<Armor>();

Weapon weapon = equipementItemBuilder
    .WithNameAndCost("Great Sword", 120)
    .WithPoints(80)
    .Build<Weapon>();

Armor chest = equipementItemBuilder
    .WithNameAndCost("Iron Chest", 200)
    .WithPoints(50)
    .WithEarth(2)
    .WithFire(3)
    .WithWater(4)
    .WithWind(3)
    .Build<Armor>();

This also looks pretty neat and as you can see the Build() method is generic which allows different types of ElementlsItems to be created with a single builder which was my original idea. The only thing I like about this design is the repetitive code :

public class BaseItemBuilder
{
    private string _name;
    private int _cost;

    public BaseItemBuilder WithNameAndCost(string name, int cost)
    {
        _name = name;
        _cost = cost;
        return this;
    }
}

And:

public class ElementalItemBuilder
{
    private string _name;
    private int _cost;

    public ElementalItemBuilder WithNameAndCost(string name, int cost)
    {
        _name = name;
        _cost = cost;
        return this;
    }
}

You can do something along the lines in the BaseItemBuilder

protected string _name;
protected int _cost;

public TBuilder WithNameAndCost<TBuilder>(string name, int cost) 
    where TBuilder : BaseItemBuilder
{
    _name = name;
    _cost = cost;
    return (TBuilder)this;
}

And later inherit the BaseItemBuilder from ElementalItemBuilder which will allow you to remove the duplicate code but you will have to specify the type argument upon invocation like this :

BaseItemBuilder itemBuilder = new BaseItemBuilder();
Item itemStone = itemBuilder
    .WithNameAndCost<BaseItemBuilder>("Stone", 1)
    .Build();

itemBuilder = new ElementalItemBuilder();
Armor helmet = itemBuilder
    .WithNameAndCost<ElementalItemBuilder>("Steel Helmet", 80)
    .WithPoints(30)
    .WithEarth(1)
    .WithFire(2)
    .Build<Armor>();

Which pretty much solves the problem with the duplicate code.

Type specific methods

Pros

  1. Simplifies the creation of the item classes.
  2. There is no duplicate code.
  3. Item properties can be set in any order.
  4. You can assign better names to each class' method.
  5. You can have some methods in let's say class Weapon but not in Armor if you want to (you can do that with the builder pattern but it will require more work).
  6. You don't need a constructor at all.

Cons

  1. Doesn't provide fully immutable objects.
  2. Requires a cast at the end of the expression (the creation of the item) or you will need to make the methods generic but this will require you specify the type argument all the time.
  3. You don't have a constructor.

Here is how it works:

public class Item
{
    public string Name { get; private set; }
    public int Cost { get; private set; }

    public override string ToString()
    {
        return "Name: " + Name + "\nCost: " + Cost;
    }

    public TITem WithNameAndCost<TITem>(string name, int cost) where TITem : Item
    {
        Name = name;
        Cost = cost;
        return (TITem)this;
    }
}

One thing I want you to take a look at here is that there is no constructor at all, where in the builder pattern we still needed the long and ugly constructors in order for the Activator to work.

Example usage:

Item itemStone = new Item()
    .WithNameAndCost<Item>("Stone", 1);

Item itemPot = new Item()
    .WithNameAndCost<Item>("Red Pot", 2);

Pretty neat and as you can tell it looks quite similar to the builder pattern.

Next up we have the elements class:

public abstract class ElementalsItem : SpecialItem
{
    public int Fire { get; private set; }
    public int Water { get; private set; }
    public int Earth { get; private set; }
    public int Wind { get; private set; }

    public override string ToString()
    {
        return base.ToString()
               + "\nFire Damages/Resistance: " + Fire
               + "\nWater Damages/Resistance: " + Water
               + "\nEarth Damages/Resistance: " + Earth
               + "\nWind Damages/Resistance: " + Wind;
    }

    //you can make those generic to avoid casting
    //but that will require you to specify the
    //type arguments all the time which is not neat.
    public ElementalsItem WithFire(int fire)
    {
        Fire = fire;
        return this;
    }

    public ElementalsItem WithWater(int water)
    {
        Water = water;
        return this;
    }

    public ElementalsItem WithEarth(int earth)
    {
        Earth = earth;
        return this;
    }

    public ElementalsItem WithWind(int wind)
    {
        Wind = wind;
        return this;
    }
}

public class Weapon : ElementalsItem
{
    public int Damages { get; private set; }
    public override int GetSpecialProperty => Damages;

    public Weapon()
    {

    }

    public Weapon WithDamage(int damagePoints)
    {
        Damages = damagePoints;
        return this;
    }
}

public class Armor : ElementalsItem
{
    public int ArmorPts { get; private set; }

    public override int GetSpecialProperty => ArmorPts;

    public Armor WithDefense(int defensePoints)
    {
        ArmorPts = defensePoints;
        return this;
    }
}

Example usage:

Armor helmet = (Armor) new Armor()
    .WithNameAndCost<Armor>("Steel Helmet", 80)
    .WithDefense(30)
    .WithEarth(1)
    .WithFire(2);

//With damage is type specific method so cast is not needed.
Weapon weapon = new Weapon()
    .WithNameAndCost<Weapon>("Great Sword", 120)
    .WithDamage(80);

Armor chest = (Armor) new Armor()
    .WithNameAndCost<Armor>("Iron Chest", 200)
    .WithDefense(50)
    .WithEarth(2)
    .WithFire(3)
    .WithWater(4)
    .WithWind(3);

Again, this looks really similar to the Builder pattern but in most cases it will require a cast which is not a big deal, but still.

You can pick whatever you like and you can of course adjust something if you don't like it.

\$\endgroup\$
12
  • \$\begingroup\$ Afair, 3 and 5 are also C# 6 features, that are not available in earlier version. \$\endgroup\$
    – Nikita B
    Commented Dec 26, 2016 at 14:39
  • \$\begingroup\$ You are correct I will update the answer to reflect the changes. \$\endgroup\$
    – Denis
    Commented Dec 26, 2016 at 14:43
  • 1
    \$\begingroup\$ I disagree on your point on the read-only getter (public virtual int GetSpecialProperty { get; }). That establishes that the property is read-only but allows inheriting classes to override the details. That doesn't have to be abstract, as abstract requires them to do so when it may not be necessary. \$\endgroup\$
    – Paul
    Commented Dec 26, 2016 at 14:48
  • \$\begingroup\$ While that's true, it's being overwritten in every derived class with a specific variable from the derived class so I assumed it's meant to be overwritten in which case it's better to be abstract. \$\endgroup\$
    – Denis
    Commented Dec 26, 2016 at 14:52
  • 1
    \$\begingroup\$ Thank you very much, now I see builder design pattern from another perspective. \$\endgroup\$
    – TKK
    Commented Dec 28, 2016 at 2:50
4
\$\begingroup\$

So, I think the biggest problem with your current solution is that it doesn't give us any context for what your trying to solve with this code, and what the classes are actually intended to do. As of right now, they're essentially data carriers with overridden ToString implementations which are not completely intention-revealing.

There's an excellent article in the comments of the linked post you had that covers a full discussion of these things here, and talks a lot about how the class structure should not be used necessarily for enforcing rules.

In any case, if it were me I would compose items from interfaces and a single base class as much as possible, eg

using System;

public class Item
{
    public Item(string name, int cost)
    {
        Name = name;
        Cost = cost;
    }
    public String Name { get; protected set;}
    public int Cost { get; protected set;}
}

public interface IWeapon
{
    int Damage {get; }
}

public interface IArmor
{
    int Resistance {get; }
}

public interface IFood
{
    int NutritionPoints { get; }
}

public interface IElement
{
    string Type { get; }
    int Strength { get; }
}

public class Spear: Item, IWeapon
{
    public Spear() : base("spear", 50)
    {
        Damage = 5; // or pass this in if you want variable damage spears
    }
    public int Damage { get; private set; }
}

public class PlateArmor: Item, IArmor
{
    public PlateArmor() : base("Plate Armor", 500)
    {
        Resistance = 50;
    }

    public int Resistance { get; private set; }
}

public class PotionOfProtection: Item, IArmor, IFood
{
    public PotionOfProtection(): base("Potion of Protection", 5000)
    {
        Resistance = 25;
        NutritionPoints = 50;
    }

    public int Resistance { get; private set; }
    public int NutritionPoints { get; private set; }
}

public class FireSpear: Item, IWeapon, IElement
{
    public FireSpear(): base("Fire Spear", 50000)
    {
        Damage = 7;
        Type = "Fire";
        Strength = 2; 
    }

    public int Damage { get; private set; }
    public string Type { get; private set; }
    public int Strength { get; private set; }
}

Behavior such as permissions and views (including the printing of things) would be handled with other classes responsible for doing so, for example a renderer class that will render information based on the type.

If an "elemental" based item can have more than one element associated with it, then I'd probably come up with another interface that covers that possibility, and in my design the 'strength' and 'type' attributes of an IElement would be evaluated by a resolver to multiply damage, resistance, etc, if the element is in play.

\$\endgroup\$
2
  • \$\begingroup\$ Well, i can agree with interfaces sense but i don't see a point of making class for every game object. \$\endgroup\$
    – TKK
    Commented Dec 27, 2016 at 9:30
  • \$\begingroup\$ You don't have to, @wdsa, I was trying to provide examples of how you'd compose classes from the interfaces. Again, without knowing your actual problem space it's difficult to give you concrete examples. \$\endgroup\$
    – Paul
    Commented Dec 27, 2016 at 13:31
3
\$\begingroup\$

The Item class should be abstract because it's rather unlikely that you'll have an instance of it. Consequently the constructor needs to be protected. The two properties this class it provides should be private as you set them in the constructor and the derived classes call base(..) there's no need for their setters to be protected. Actually you can remove the setters if you are on C# 6.

The same rules apply to other classes derived from the Item or other items.


The GetSpecialProperty sounds a like a workaround or a property for everything and nothing. ToString suggests it has at least three purposes:

  • Damages
  • ArmorPts
  • HungryRestorePts

This is not a good idea. Try to implement those properties on the appropriated objects instead of having one that nobody knows what it actually means.

Side note: Don't abbreviate the names. Use Points and not Pts. It's the only

But I see have even more multipupose properties e.g. the ElementsItem has a Fire property and ToString says it stands for:

  • Damages
  • Resistance

You should really split this or give them proper names like FireResistance. How do you know which one is it actually?

I think you should create a new class Element and derive the Fire etc. from it so that you can use a dictionary or an array of elements.


\n

You should use the Environment.NewLine property or the StringBuilder.AppendLine method if there is no real reason for the \n.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ I don't agree with your opinion about about that Item should be abstract, because sometimes I will want to create objects that will have only basic properties like exactly name and cost, like stone or some jewellery. And I see that all of you are using Enviroment.NewLine, why "\n" is bad for this? :) \$\endgroup\$
    – TKK
    Commented Dec 26, 2016 at 18:14
3
\$\begingroup\$

I think these are nice solutions if you are into design patterns. But it does seem to make the problem more difficult than it needs to be.

What is an item? A collection of characteristics. Designing a class for every possible characteristic is weird. My thinking is why? Moreover, why would you want to perform a type check for every characteristic of an item? This may be fine in some games, but suppose you were designing an arpg, See Here. This would become flat out inefficient.

Another approach could be to define specific items.

public class Item
{
    public readonly string Name;
    public readonly string Description;
    public readonly string ID;
    public readonly string Type;
    public readonly float Value;
    public readonly float Weight;
    public readonly bool IsStackable;

    private Item() { }

    public Item(string name, string description, string id, string type, float value, float weight, bool isStackable)
    {
        Name = name;
        Description = description;
        ID = id;
        Type = type;
        Value = value;
        Weight = weight;
        IsStackable = isStackable;
    }
}

This way you could have an item, such as a cup, which may not serve any purpose other than random loot/props that have value for the player to sell. You could then define item type such as equipment. Is stackable is something you will have to determine, considering that not all items are stackable, but you may want the previously mentioned cup to be stackable.

public class Equiptable : Item
{
    public readonly SlotType Slot;
    private List<Modifier> _Modifiers;
    public IList<Modifier> Modifiers { get { return _Modifiers.AsReadOnly(); } }

    public Equiptable(string name, string description, string id, string type, float value, float weight, bool isStackable, SlotType slot, List<Modifier> modifiers) 
        : base(name, description, id, type, value, weight, isStackable)
    {
        Slot = slot;
        _Modifiers = modifiers;
    }

    public void AddModifier(Modifier modifier)
    {
        _Modifiers.Add(modifier);
    }

    public void RemoveModifier(Modifier modifier)
    {
        _Modifiers.Remove(modifier);
    }
}

Here we define an equitable item. An equitable item is going to provide some benefit to the player - when equipped. An equitable is a collection of modifiers that provide some sort of modifier to the player when equipped.

So what is a modifier? Well it is just a name, value, and calculation. The calculation determines how the modifier will be applied. Such as a flat rate or percentage. They are just flags, and can be represented as an enum. Note, there may be more than one percent calculation, as mathematically percentages display unique properties when added together or applied individually.

/// <summary>
/// Represents a Modifier
/// </summary>
public class Modifier
{
    #region readonly members
    /// <summary>
    /// This modifier's name
    /// </summary>
    public readonly string Name;
    /// <summary>
    /// This modifier's description
    /// </summary>
    public readonly string Description;
    /// <summary>
    /// This modifier's id
    /// </summary>
    public readonly string ID;
    /// <summary>
    /// This modifier's type
    /// </summary>
    public readonly string Type;
    /// <summary>
    /// This modifiers magnitudue
    /// </summary>
    public readonly float Magnitude;
    /// <summary>
    /// This modifier's calculations type
    /// </summary>
    public readonly CalculationFlag Calculation;
    #endregion readonly members

    #region constructors
    /// <summary>
    /// prevent public access to unititialized modifier
    /// </summary>
    private Modifier() { }

    /// <summary>
    /// Constructs a new modifier
    /// </summary>
    /// <param name="name">This modifier's name</param>
    /// <param name="descrpition">This modifier's description</param>
    /// <param name="id">This modifier's id</param>
    /// <param name="type">This modifier's type</param>
    /// <param name="mangnitude">This modifier's magnitude</param>
    /// <param name="calculation">This modifier's calculation type</param>
    public Modifier(string name, string descrpition, string id, string type, float mangnitude, CalculationFlag calculation)
    {
        //
        //  Initialize member variables
        //
        Name = name;
        Description = descrpition;
        ID = id;
        Type = type;
        Magnitude = mangnitude;
        Calculation = calculation;
    }

    #endregion constructors
}

Forgive the comments, this is copied from active code. However, it may provide intuition as to what is going on. In addition to the values I had mentioned, the modifier is just a memory location with values. So, what do these modifiers actually do? How do they work? The type, as I have it will define the attribute/stat that it effects. It is a string to so I may define all my modifiers in a file loaded into the program. Many benefits to this, but the one I am interested in is the data driven programming aspect. I will not need to recompile to modify a modifier. Moreover, a nice thing about this is that I can define a normal item as a minimum and maximum damage modifier. The modifiers will apply to the character, when equipped, like any other. We can then have an attribute class to represent stats for a character.

Then we have the attribute

    /// <summary>
/// Represents an Attribute
/// </summary>
public class Attrbute
{
    #region public variables
    /// <summary>
    /// An action to take place when the base value is changed
    /// </summary>
    public Action OnBaseValueChange;
    /// <summary>
    /// An action to take place when the current value is changed
    /// </summary>
    public Action OnCurrentValueChange;

    /// <summary>
    /// This attribute's name
    /// </summary>
    public readonly string Name;
    /// <summary>
    /// This attribute's description
    /// </summary>
    public readonly string Description;
    /// <summary>
    /// This attribute's id
    /// </summary>
    public readonly string ID;
    #endregion public variables

    #region private variables
    /// <summary>
    /// The base value of this attribute
    /// </summary>
    private float _BaseValue;
    /// <summary>
    /// The current value of this attribute
    /// </summary>                
    private float _CurrentValue;  
    /// <summary>                 
    /// The modified state of this attribute
    /// </summary>
    private bool _IsModified;

    /// <summary>
    /// A collection of modifiers applied to this attribute
    /// </summary>
    private List<Modifier> _Modifiers;
    #endregion private variables

    #region public properties
    /// <summary>
    /// A read-only collection of modifiers applied to this attribute
    /// </summary>
    public IList<Modifier> Modifiers { get { return _Modifiers.AsReadOnly(); } }

    /// <summary>
    /// The base value of this attribute
    /// </summary>
    public float BaseValue
    {
        get
        {
            //return the base value
            return _BaseValue;
        }
        set
        {
            //update the base value
            _BaseValue = value;
            //Update modified state
            _IsModified = true;

            if (OnBaseValueChange != null)
                OnBaseValueChange();
        }
    }

    /// <summary>
    /// Returns the current value of this modifier
    /// </summary>
    public float CurrentValue
    {
        get
        {
            //update the current value if this attribute has been modified
            if (_IsModified)
            {
                //get new current value
                _CurrentValue = CalculateCurrentValue();
                //update modified state to false
                _IsModified = false;

                if (OnCurrentValueChange != null)
                    OnCurrentValueChange();
            }
            //return the current value
            return _CurrentValue;
        }
    }

    #endregion public properties


    #region constructors
    /// <summary>
    /// Prevents public access to instantiate an unitialized attribute
    /// </summary>
    private Attrbute() { }

    /// <summary>
    /// Constructs an instance of an attribute with an empty collection of modifiers
    /// </summary>
    /// <param name="name">The name of the attribute</param>
    /// <param name="description">The description of the attribute</param>
    /// <param name="id">The id of the attribute</param>
    /// <param name="baseValue">The base value of the attribute</param>
    public Attrbute(string name, string description, string id, float baseValue)
    :this(name, description, id, baseValue, new List<Modifier>()) { }   //TODO(stephen): Performance -> A new list with an initialze size?

    /// <summary>
    /// Constructs an attribute with a given collection of modifiers
    /// </summary>
    /// <param name="name">The name of the attribute</param>
    /// <param name="description">The description of the attribute</param>
    /// <param name="id">The id of the attribute</param>
    /// <param name="baseValue">The base value of the attribute</param>
    /// <param name="modifiers">The collection of modifiers for the attribute</param>
    public Attrbute(string name, string description, string id, float baseValue, List<Modifier> modifiers)
    {
        //
        //Initialize member variables
        //
        Name = name;
        Description = description;
        ID = id;
        BaseValue = baseValue;
        _Modifiers = modifiers;
    }

    #endregion constructors


    #region public methods
    /// <summary>
    /// Adds a modifier to this attribute
    /// </summary>
    /// <param name="modifier">The attribute to remove</param>
    public void AddModifier(Modifier modifier)
    {
        //Add modifier to collection
        _Modifiers.Add(modifier);
        //update modified state
        _IsModified = true;
    }

    /// <summary>
    /// Removes a modifier from this attribute
    /// </summary>
    /// <param name="modifier">The modifier to remove</param>
    public void RemoveModifier(Modifier modifier)
    {
        //Remove modifier to collection
        _Modifiers.Remove(modifier);
        //update modified state
        _IsModified = true;
    }
    #endregion public memthods

    #region private properties
    /// <summary>
    /// Calculates the current value from all applied modifiers
    /// </summary>
    /// <returns></returns>
    private float CalculateCurrentValue()
    {
        //initiliaze local variables
        float flatRateValues = BaseValue;
        float percentRateValue = 1.0f;

        //iterate through all modifiers applied to this attribute
        foreach (Modifier modifier in Modifiers)
        {
            //add magnitude to flat rate values
            if (modifier.Calculation == CalculationFlag.Flat)
                flatRateValues += modifier.Magnitude;
            //add magnitude to percent rate values
            else if (modifier.Calculation == CalculationFlag.Percent)
                percentRateValue += modifier.Magnitude;
            //calculation type not found, throw exception
            else
                throw new NotSupportedException("Attribute->CalculateCurrentValue() - CalcuationType -> "
                    + modifier.Calculation + " Not Found!");
        }

        //return the calculated value
        return flatRateValues * percentRateValue;
    }
    #endregion private variables
}

The attribute is relatively strait forward. We manage a collection of modifiers that apply to the attribute. The actions provide means of doing things when the attribute is modified, such as updating ui or some other attribute, such as max health.

Now all we need is a character that can equip equitables.

public class Character
{
    private List<Attrbute> _Attributes;
    private Dictionary<SlotType, Equiptable> _Equiptment;

    public IList<Attrbute> Attributes { get { return _Attributes.AsReadOnly(); } }

    public Character(List<Attrbute> attributes)
    {
        _Attributes = attributes;
        _Equiptment = new Dictionary<SlotType, Equiptable>();

        foreach (SlotType slot in System.Enum.GetValues(typeof(SlotType)))
        {
            _Equiptment.Add(slot, null);
        }
    }

    public Attrbute FindAttribute(string name)
    {
        return _Attributes.Find(attribute => attribute.Name == name);
    }

    public void Equipt(Equiptable equiptable)
    {
        Equiptable currentEquipt = _Equiptment[equiptable.Slot];

        _Equiptment[equiptable.Slot] = equiptable;

        foreach (Modifier modifier in equiptable.Modifiers)
        {
            Attrbute attribute = FindAttribute(modifier.Type);

            if (attribute != null)
                attribute.AddModifier(modifier);
        }

        //add currentEquipt to inventory?
    }

    public void UnEquipt(SlotType slot)
    {
        Equiptable removed = _Equiptment[slot];
        _Equiptment[slot] = null;

        foreach (Modifier modifier in removed.Modifiers)
        {
            Attrbute attribute = FindAttribute(modifier.Type);

            if (attribute != null)
                attribute.RemoveModifier(modifier);
        }
        //add removed to inventory?
    }
}

Equitable slots is an enum that you can define however you like, but int this example:

public enum SlotType
{
    None = 0x0000,
    Primary = 0x0001,
    Secondary = 0x0002,
    TwoHanded = 0x0004,
    Ranged = 0x0008,
}

All of this is just a suggestion. Each game will handle things differently. For instance, you may not have props as items, such as a cup. Moreover, the desing patterns mentioned in this post may serve you well. It all comes down to how you want to manage your items. If you wanted crafting of some kind with raw resources, then you can define a class for that.

\$\endgroup\$

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