Mini RPG equipment code

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.

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.

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.

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.