Classes representing items in an RPG game

Question

I wrote a little program in C# that contains classes representing Items in RPG game. I wanted to have access to all inherited classes parameters from list contains parent Item class instances, so this is my code:

using System;
using System.Collections.Generic;
public interface SpecialObject
{
 int GetIntParameter
 {
 get;
 }
}
public interface ElementalsObject
{
 int[] GetElementalsParameters
 {
 get;
 }
}
public class Item
{
 private string _name;
 private int _cost;
 public string name
 {
 get { return _name; }
 }
 public int cost
 {
 get { return _cost; }
 }
 public Item(string name, int cost)
 {
 _name = name;
 _cost = cost;
 }
}
public class Weapon : Item, SpecialObject, ElementalsObject
{
 private int dmgs;
 private int[] elementalsDmgs;
 public int GetIntParameter
 {
 get { return dmgs; }
 }
 public int[] GetElementalsParameters
 {
 get { return elementalsDmgs; }
 }
 public Weapon(string name, int cost, int dmgs, int[] elementalsDmgs = null) : base(name, cost)
 {
 if (elementalsDmgs != null)
 this.elementalsDmgs = elementalsDmgs;
 else
 this.elementalsDmgs = new int[] { 0, 0, 0, 0 };
 this.dmgs = dmgs;
 }
}
public class Armor : Item, SpecialObject, ElementalsObject
{
 private int armorPts;
 private int[] elementalsArmorPts;
 public int GetIntParameter
 {
 get { return armorPts; }
 }
 public int[] GetElementalsParameters
 {
 get { return elementalsArmorPts; }
 }
 public Armor(string name, int cost, int armorPts, int[] elementalsArmorPts = null) : base(name, cost)
 {
 if (elementalsArmorPts != null)
 this.elementalsArmorPts = elementalsArmorPts;
 else
 this.elementalsArmorPts = new int[] { 0, 0, 0, 0 };
 this.armorPts = armorPts;
 }
}
public class Food : Item, SpecialObject
{
 private int _hungryRestorePts;
 public int GetIntParameter
 {
 get { return _hungryRestorePts; }
 }
 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, new int[4] {0, 0, 0, 60}),
 new Weapon("Fire Spear", 1400, 60, new int[4] {0, 80, 0, 0}),
 new Food("Apple", 5, 10) });
 string[] el = new string[4] { "Water", "Fire", "Earth", "Wind" };
 foreach(Item i in items)
 {
 Console.WriteLine("Name: " + i.name + ", Cost: " + i.cost);
 if(i is SpecialObject)
 {
 //SpecialObject so = i as SpecialObject;
 SpecialObject so = (SpecialObject)i;
 Console.WriteLine(" Damages/ArmorPts/HungryRestorePts: " + so.GetIntParameter);
 }
 if(i is ElementalsObject)
 {
 ElementalsObject eo = i as ElementalsObject;
 Console.WriteLine(" Elementals parameters: ");
 for (int e = 0; e < el.Length; e++)
 {
 Console.WriteLine(" " + el[e] + ": " + eo.GetElementalsParameters[e] + ", ");
 }
 }
 Console.WriteLine();
 }
 }
}

What do you think about it? Is it good code? If no then how I can write it better? :)

• Eric Lippert: You might be interested to read ericlippert.com/2015/04/27/wizards-and-warriors-part-one
• Simon Forsberg: Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers .
• TKK: Ok, sorry, I put code in answer.

Answer 1

public interface SpecialObject
{
 int GetIntParameter
 {
 get;
 }
}
public interface ElementalsObject
{
 int[] GetElementalsParameters
 {
 get;
 }
}

1. What does "object" mean? If it refers to a domain concept, then it should have a more specific name (any "thing" can be an "object"); if it's a technical term, then it should be removed (all classes are objects, no need to say so explicitly).
2. GetIntParameter and GetElementalsParameters are pretty poor names. What does the parameter represent? Name the property after that, not after how it may be supplied to the implementing class.

public class Item
{
 private string _name;
 private int _cost;
 public string name
 {
 get { return _name; }
 }
 public int cost
 {
 get { return _cost; }
 }
 public Item(string name, int cost)
 {
 _name = name;
 _cost = cost;
 }
}

This is ok although it doesn't really do much (or anything), just a class holding two properties. You could make the code shorter by writing it like this instead. And remember public properties should be PascalCase.

public class Item
{
 public string Name { get; private set; } // private set only required if not using C# 6.0
 public int Cost { get; private set; }
 public Item(string name, int cost)
 {
 Name = name;
 Cost = cost;
 }
}

So now we see a class implementing your interfaces.

public class Weapon : Item, SpecialObject, ElementalsObject
{
 private int dmgs;
 private int[] elementalsDmgs;
 public int GetIntParameter
 {
 get { return dmgs; }
 }
 public int[] GetElementalsParameters
 {
 get { return elementalsDmgs; }
 }
 public Weapon(string name, int cost, int dmgs, int[] elementalsDmgs = null) : base(name, cost)
 {
 if (elementalsDmgs != null)
 this.elementalsDmgs = elementalsDmgs;
 else
 this.elementalsDmgs = new int[] { 0, 0, 0, 0 };
 this.dmgs = dmgs;
 }
}

First of all the constructor can be shortened by assigning elementalsDmgs like so: this.elementalsDmgs = elementalsDmgs ?? new int[] { 0, 0, 0, 0 };.

Now you see how the property names on your interfaces are unhelpful. What does GetIntParameter mean or return on a Weapon, Armor, or Food object, aside from "an int"? Who knows. What is the meaning of the values that GetElementalsParameters returns on Weapon and Armor? Well I know it returns something to do with "elementals" (whatever that is - maybe that word makes sense in your domain), but who knows the meaning of the parameters?

The much bigger problem is that GetIntParameter actually has no meaning; your classes have int parameters, so you created an interface to represent it so you can get the value from all classes implementing that interface. But on Food and Armor it contains a number of "points", and on Weapon it refers to "damages". (Assuming I'm interpreting "dmgs" correctly - which is another issue, don't skip the vowels! "dmgs" only makes sense to you, everyone else has to guess what it might mean.) The fact that they're always int does not make them in any way related, and is not a shared behaviour or functionality.

class Equipment
{
 public static void Test()
 {
 ...
 string[] el = new string[4] { "Water", "Fire", "Earth", "Wind" };
 ...
 Console.WriteLine(" Elementals parameters: ");
 ...
 Console.WriteLine(" " + el[e] + ": " + eo.GetElementalsParameters[e] + ", ");

Ah, so the elementals parameters are "Water", "Fire", "Earth", "Wind". So the ElementalsObject should have properties with those names, each of which can be assigned an int, rather than a property with an int array. I mean, who can guarantee that the values of the int array were even supplied in the correct order?

if(i is SpecialObject)
 SpecialObject so = (SpecialObject)i;
if(i is ElementalsObject)
 ElementalsObject eo = i as ElementalsObject;

It's not clear why you do this in two different ways just a few lines apart, but neither is the best way to do it as both involve casting twice. What you should do is this, as it only casts once:

 var o = i as SpecialObject;
 if (o != null)
 // cast is valid.

---

I don't really want to take the time to rewrite the code myself as this doesn't feel like "real" code, more like an experiment. But some suggestions are:

• Rename SpecialObject to something clearer; rename GetIntParameter to something to do with points.
• Rename the ElementalsObject interface to something clearer; remove current property and replace with properties of actual names (Fire, Water, etc).
• Consider whether the above should even be interfaces. To me this calls for a base class for these different types of "items", so you don't have to implement the interface properties on each type of item.
• Use a factory for item creation. Creating an item like so new Armor("Storm Cuirass", 1000, 120, new int[4] {0, 0, 0, 60}) is no good as it's too time consuming and error prone.
• You shouldn't have to say if (i is SpecialObject) and if (i is ElementalsObject). Use polymorphism to call a method on the objects which returns the correct result per object type. (In this case, you could simply override ToString() in the classes, but again this requires a proper base class structure.)

• TKK: Thanks for that exhausting answer, I will rewrite code in a way that you showed here. :)
• TKK: Can you tell me more about last point of your suggestions? What i have to put into ToString function?
• 404: The ToString is an example for your specific usage; i.e. since you are only using the objects in Console.Writeline, you could for example override ToString in a base class to return the string "Name: " + Name + ", Cost: " + Cost; then when you create a base class implementing points, you can override ToString something like: return base.ToString() + "\nDamages/ArmorPts/HungryRestorePts: " + Points; etc. Then when you iterate through the items, you can just say Console.Writeline(item) and it will print the item details in the way you want and with all the specifics for that item type.
• TKK: I changed code, what do you think now?