3
\$\begingroup\$

I'm making a game in Unity and I need to have a inventory/item system, I decided to store my items in simple XML document which I later read from. I'm planning to have different varieties of items and they will all have distinct classes which extend they're base interface. Here's how my hierarchy looks like for now :

IBaseItem

public interface IBaseItem
{
    int ItemID { get; }
    string ItemName { get; }
    string Description { get; }
}

A really simple interface which contains all the common properties between all the items.

EquipementItem : IBaseItem

[System.Serializable]
public abstract class EquipementItem : IBaseItem
{
    public enum ItemTypes
    {
        None,
        Armor,
        Weapon
    }

    public enum SlotTypes
    {
        Head,
        Shoulders,
        Chest,
        Bracers,
        Gloves,
        Waist,
        Legs,
        Boots,
        Weapon
    }

    private readonly int itemID;
    public int ID
    {
        get { return itemID; }
    }

    private readonly string itemName;
    public string Name
    {
        get { return itemName; }
    }

    private readonly string description;
    public string Description
    {
        get { return description; }
    }

    private readonly ItemTypes itemType;
    public ItemTypes ItemType
    {
        get { return itemType; }
    }

    private readonly SlotTypes slotType;
    public SlotTypes SlotType
    {
        get { return slotType; }
    }

    protected EquipementItem(int itemID, string itemName, ItemTypes itemType, SlotTypes slotType, string description)
    {
        this.itemID = itemID;
        this.itemName = itemName;
        this.itemType = itemType;
        this.slotType = slotType;
        this.description = description;
    }
}

This is the class for all the equippable items, and since the items are constant throughout the entire game I made the object immutable. However this might cause me some trouble since the constructor apparently needs too much parameters (there will be more) and I wont be able implement the Builder Pattern because of the readonly fields. I also wonder if I need this class to be made abstract and then inherited by other classes such as Weapon & Armor for example, but I don't see any different in the implementation of those 2 so for now I have it just as a normal class.

ItemDatabase.xml

  <EquipementItem>
    <ID>1</ID>
    <Name>Basic Sword</Name>
    <ItemType>Weapon</ItemType>
    <SlotType>Weapon</SlotType>
    <Description>A basic sword</Description>
  </EquipementItem>

  <EquipementItem>
    <ID>2</ID>
    <Name>Advanced Sword</Name>
    <ItemType>Weapon</ItemType>
    <SlotType>Weapon</SlotType>
    <Description>Not a basic sword</Description>
  </EquipementItem>

</EquipementItems>

That's pretty much how the db will look like with a few additional tags

DatabaseBuilderEQItems : MonoBehaviour

public class DatabaseBuilderEQItems : MonoBehaviour
{
    public TextAsset DatabaseFile;

    public static List<EquipementItem> EquipementItems;

    private void Awake()
    {
        if(EquipementItems == null)
        {
            EquipementItems = new List<EquipementItem>();
        }
        LoadEquipementItemsFromDB();
    }

    private void LoadEquipementItemsFromDB()
    {
        XmlDocument xmlDocument = new XmlDocument();
        xmlDocument.LoadXml(DatabaseFile.text);
        XmlNodeList equipementItemList = xmlDocument.GetElementsByTagName("EquipementItem");

        foreach (XmlNode equipementItem in equipementItemList)
        {
            XmlNodeList itemInfo = equipementItem.ChildNodes;

            EquipementItem item = new EquipementItemParser(
                itemInfo.Cast<XmlNode>()
                    .ToDictionary(itemContent => itemContent.Name, itemContent => itemContent.InnerText));

            EquipementItems.Add(item);
        }
    }
}

This is where I read and save the items from the database.

EquipementItemParser : EquipementItem

public class EquipementItemParser : EquipementItem
{
    public EquipementItemParser(IDictionary<string, string> databaseItemInfo)
        : base(
            int.Parse(databaseItemInfo["ID"]), databaseItemInfo["Name"],
            (ItemTypes) Enum.Parse(typeof(ItemTypes), databaseItemInfo["ItemType"]),
            (SlotTypes) Enum.Parse(typeof(SlotTypes), databaseItemInfo["SlotType"]),
            databaseItemInfo["Description"])
    {

    }
}

What bothers me the most is that with my current setup where I read from the xml document if I create a new type of items I will pretty much need to rewritte a new script with almost the exact same code with the small difference of few types namely EquipementItem & EquipementItemParser can anything be done about that ?

Are there any flaws in my design ?

How extendable is this design ?

And overall what can be improved ?

\$\endgroup\$
2
\$\begingroup\$

Suggesting alternatives is always a risky review (and usually one gets DV for this) but I'll try it anyway...


Nowaydays I wouldn't chose XML anymore. With JSON you'd be better off. The most popular framework (JSON.NET) can solve all your problems because:

  • you don't have to worry about parsing it
  • you don't have to worry about converting any types - it'll handle everything for you - all enums, ints, doubles or any custom types.

If you decorate your type's constructor with the JsonConstructorAttribute it can even handle immutable objects .


This means that the loading and saving part would virtually be reduced to just a few lines:

var json = // read json from db
var equipementItems = JsonConvert.DeserializeObject <EquipementItem[]>(json);

and

var json = JsonConvert.SerializeObject(equipementItems);

JsonConvert.DeserializeObject Method

JsonConvert.SerializeObject Method

\$\endgroup\$
  • \$\begingroup\$ This is a great suggestion mate I will make sure to check it out when Im back home and I will update you on the results . \$\endgroup\$ – Denis Nov 24 '16 at 8:08
  • \$\begingroup\$ But do you mean reading the Xml document using JSON or I should create a separate JSON file similar to the existing XML one ? \$\endgroup\$ – Denis Nov 24 '16 at 9:18
  • \$\begingroup\$ @denis I mean using only JSON. I've read that you could use XML but if you have a choice and can use JSON then I find this is a much better option. \$\endgroup\$ – t3chb0t Nov 24 '16 at 9:21
  • \$\begingroup\$ Thank you for pointing me to JSON I'm currently using JSONUtility which is a class defined in the default UnityEngine namespace it works really fast and solves all the problems just as you said. P.S Don't be afraid to post alternative suggestions you just saved me so much time. ^^ \$\endgroup\$ – Denis Nov 25 '16 at 18:59
1
\$\begingroup\$

I'm looking at the way you've implemented the properties on EquipementItem. Here's what you currently have:

private readonly int itemID;
public int ID
{
    get { return itemID; }
}

private readonly string itemName;
public string Name
{
    get { return itemName; }
}

private readonly string description;
public string Description
{
    get { return description; }
}

private readonly ItemTypes itemType;
public ItemTypes ItemType
{
    get { return itemType; }
}

private readonly SlotTypes slotType;
public SlotTypes SlotType
{
    get { return slotType; }
}

Here's an alternative way to implement the properties, which is much more concise:

public int ID { get; private set; }
public string Name { get; private set; }
public string Description { get; private set; }
public ItemTypes ItemType { get; private set; }
public SlotTypes SlotType { get; private set; }

Implementing the properties this way will allow you to use the builder pattern, because nested classes of EquipementItem can use its private members.

(By the way, you've misspelled the word "equipment".)


The way that you've implemented the EquipementItemParser class feels a little strange. By creating a subclass of EquipementItem, you're saying that you have something which is a special type of EquipementItem. But there isn't really anything special about an equipment item which is created based on an IDictionary.

Consider making the EquipementItem class non-abstract and making its constructor public. If you do this, then you can write this static method somewhere:

public static ParseEquipementItem(IDictionary<string, string> databaseItemInfo)
{
    int itemID = int.Parse(databaseItemInfo["ID"]);
    string itemName = databaseItemInfo["Name"];
    ItemTypes itemType = (ItemTypes) Enum.Parse(typeof(ItemTypes), databaseItemInfo["ItemType"]);
    SlotTypes slotType = (SlotTypes) Enum.Parse(typeof(SlotTypes), databaseItemInfo["SlotType"]);
    string description = databaseItemInfo["Description"];

    return new EquipementItem(itemID, itemName, itemType, slotType, description);
}

What bothers me the most is that with my current setup where I read from the xml document if I create a new type of items I will pretty much need to rewritte a new script with almost the exact same code with the small difference of few types namely EquipementItem & EquipementItemParser can anything be done about that ?

Yes, I think so! This sounds like the problem that XML serialization is designed to solve. See the MSDN article "Introducing XML Serialization".

\$\endgroup\$
  • \$\begingroup\$ Having my properties like this makes my object no longer fully immutable. \$\endgroup\$ – Denis Nov 24 '16 at 5:23
  • \$\begingroup\$ @denis It looks to me like the object is still fully immutable in practice (as long as the setters are never called outside of the constructor). Do you disagree? \$\endgroup\$ – Tanner Swett Nov 24 '16 at 7:43
  • \$\begingroup\$ The fact that you are able to change some value outside of the constructor ( the inital valuea of the object ) means it is no longer a immutable object. \$\endgroup\$ – Denis Nov 24 '16 at 8:07
  • \$\begingroup\$ With C# 6.0 (using VS 2015) you can assign values in the constructor to "readonly" (having only a getter) properties. \$\endgroup\$ – Heslacher Nov 24 '16 at 8:39
  • \$\begingroup\$ Sadly Unity is not using C# 6.. \$\endgroup\$ – Denis Nov 24 '16 at 9:17

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.