Dynamic Storing Objects from XML in RPG

Question

I would prefer if more experienced users could give pointers on how I can optimize and think better when writing code.

If you are unfamiliar with unity3d, ignore the use of UnityEngine, the heritage from MonoBehaviour as well as the Debug.Log();, Debug.LogWarning();, and Debug.LogError();

Awake is called directly after the constructor.

I use int length instead of a function to return the size of List<Gender> genders. Not sure what is preferred (or best).

An XML Example can be seen further down.

/// <summary> /// Gender manager. /// Access length by GenderManager.Length /// Access gender by index GenderManager.gender[int] /// /// Left to do: Singleton /// /// Author: Emz /// Date: 2014-01-21 /// </summary> using UnityEngine; using System.Collections; using System.Collections.Generic; using System.Xml; using System; public class GenderManager : MonoBehaviour { private static List<Gender> genders; private static int length; // Use this for initialization public GenderManager () { genders = new List<Gender> (); length = 0; } void Awake () { DontDestroyOnLoad (this); XmlDocument doc = new XmlDocument (); doc.Load (@"genders.xml"); XmlNodeList gs = doc.SelectNodes ("genders/gender"); foreach (XmlNode g in gs) { Gender tg = new Gender (); tg.Name = g.SelectSingleNode("name").InnerText; tg.Desc = g.SelectSingleNode("desc").InnerText; XmlNodeList ams = g.SelectNodes("attributemodifiers/attributemodifier"); foreach (XmlNode am in ams) { // check if attribute does exist in public enum AttributeName under Skill.cs if (Enum.IsDefined(typeof(AttributeName), am.SelectSingleNode ("attribute").InnerText)) { int ta = (int)Enum.Parse(typeof(AttributeName), am.SelectSingleNode ("attribute").InnerText); // returns 0 if conversion failed int tv = Convert.ToInt32(am.SelectSingleNode ("value").InnerText); tg.AddAttributeModifier (ta, tv); // if attribute does not exist in SkillName under Skill.cs } else { Debug.LogError ("Invalid Attribute Name: " + am.SelectSingleNode ("attribute").InnerText); } } XmlNodeList sms = g.SelectNodes("skillmodifiers/skillmodifier"); foreach (XmlNode sm in sms) { // check if skill does exist in public enum SkillName under Skill.cs if (Enum.IsDefined(typeof(SkillName), sm.SelectSingleNode ("skill").InnerText)) { int ts = (int)Enum.Parse(typeof(SkillName), sm.SelectSingleNode ("skill").InnerText); // returns 0 if conversion failed int tv = Convert.ToInt32(sm.SelectSingleNode ("value").InnerText); tg.AddSkillModifier (ts, tv); // if skill does not exist in SkillName under Skill.cs } else { Debug.LogError ("Invalid Skill Name: " + sm.SelectSingleNode ("skill").InnerText); } } // off we go, increment length genders.Add (tg); ++length; } } public static int Length { get {return length;} } public static Gender Gender (int index) { return genders [index]; } } 

XML

<?xml version="1.0" encoding="UTF-8"?> <genders> <gender> <name>Female</name> <desc>FemDesc</desc> <attributemodifiers> <attributemodifier> <attribute>Agility</attribute> <value>1</value> </attributemodifier> </attributemodifiers> <skillmodifiers> <skillmodifier> <skill>Charm</skill> <value>1</value> </skillmodifier> </skillmodifiers> </gender> <gender> <name>Male</name> <desc>MalDesc</desc> <attributemodifiers> <attributemodifier> <attribute>Strength</attribute> <value>1</value> </attributemodifier> </attributemodifiers> <skillmodifiers> <skillmodifier> <skill>Intimidation</skill> <value>1</value> </skillmodifier> </skillmodifiers> </gender> <gender> <name>Neuter</name> <desc>NeuDesc</desc> <attributemodifiers> <attributemodifier> <attribute>Attunement</attribute> <value>1</value> </attributemodifier> </attributemodifiers> <skillmodifiers> <skillmodifier> <skill>Coercion</skill> <value>1</value> </skillmodifier> </skillmodifiers> </gender> </genders> 

Enums for AttributeName and SkillName

public enum AttributeName { Strength, Agility, Quickness, Endurance, Attunement, Focus }; public enum SkillName { Weight_Capacity, Attack_Power, Intimidation, Coercion, Charm }; 

And lastly, the Gender class

using System.Collections.Generic; public class Gender { private string _name; private string _desc; private List<GenderBonusAttribute> _attributeMods; private List<GenderBonusSkill> _skillMods; public Gender () { _name = string.Empty; _attributeMods = new List<GenderBonusAttribute> (); _skillMods = new List<GenderBonusSkill> (); } public string Name { get {return _name;} set {_name = value;} } public string Desc { get {return _desc;} set {_desc = value;} } public void AddAttributeModifier (int a, int v) { _attributeMods.Add (new GenderBonusAttribute (a, v)); } public void AddSkillModifier (int s, int v) { _skillMods.Add (new GenderBonusSkill (s, v)); } public List<GenderBonusAttribute> AttributeMods { get {return _attributeMods;} } public List<GenderBonusSkill> SkillMods { get {return _skillMods;} } } public class GenderBonusAttribute { public int attribute; public int value; public GenderBonusAttribute (int a, int v) { attribute = a; value = v; } } public class GenderBonusSkill { public int skill; public int value; public GenderBonusSkill (int s, int v) { skill = s; value = v; } } 

I don't want to hardcode the genders for various reasons.

Does this code look good enough? If not, what changes should be made and where can I read more about them?

Answer 1

1. Your code style is really inconsistent. As if you were copypasting code blocks from various places and didn't bother to refactor it. Part of the reason your code is hard to read. A somewhat accepted code style is: a) prefix field names with underscores, b) if possible use auto-properties for public members instead of fields and properties with backing fields
2. public GenderBonusAttribute (int a, int v) what is a? what is v? no way to tell without digging into your code. You should use descriptive names.
3. Using static fields in GenderManager and setting them via non-static constructor is a mess.
4. Length is not descriptive. What is length? If it is the length of genders, then why dont you expose genders.Count instead?
5. In my opinion XmlDocument is somewhat depricated. I would use XDocument and Linq-to-Xml instead. It would simplify your code. Though in a sense its probably a matter of taste.
6. I think some light weight data base will do a better job in storing game mechanics then xml files.

Answer 2

In addition to Nik's comments:

If you are not married to the structure of your XML document and are happy to change it then a simpler method would be to use the XmlSerializer

Using it your GenderManager can be de/serialized easily by a few lines of code:

public class GenderManager { public List<Gender> Genders { get; private set; } // Use this for initialization public GenderManager () { Genders = new List<Gender>(); } public static GenderManager Deserialize() { using (var reader = new StreamReader("genders.xml")) { return (GenderManager)new XmlSerializer(typeof(GenderManager)).Deserialize(reader); } } public void Serialize() { using (var writer = new StreamWriter("genders.xml")) { new XmlSerializer(typeof(GenderManager)).Serialize(writer, this); } } public int Length { get {return Genders.Count;} } public Gender Gender (int index) { return Genders [index]; } } 

Slight catch:

1. All serialized types need to have a parameterless constructor although it is enough for it to be protected.
2. All properties and fields you wish to be serialized need to be public. The serializer will ignore private properties and fields.

Note: I have taken the liberty to remove the static from all fields and just return the Count of the list rather than keeping track of the length separately.

When serialization requirements get more complex it can get problematic but for simple stuff it's hard to beat.

Sample XML file:

<?xml version="1.0" encoding="utf-8"?> <GenderManager xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema"> <Genders> <Gender> <Name /> <AttributeMods> <GenderBonusAttribute> <attribute>0</attribute> <value>10</value> </GenderBonusAttribute> </AttributeMods> <SkillMods /> </Gender> <Gender> <Name /> <AttributeMods /> <SkillMods> <GenderBonusSkill> <skill>4</skill> <value>5</value> </GenderBonusSkill> </SkillMods> </Gender> </Genders> </GenderManager> 

Some other remarks:

1. You should use descriptive names for variables and parameters. Descriptive names go a long way of describing the purpose of a variable or parameter which can greatly improve readability and maintainability. See them as built-in documentation.

2. You have defined enums for attribute and skill names so why do you use an int to add them to your object. They should be using the enum types, e.g.:

   public class GenderBonusAttribute { public AttributeName attribute; public int value; public GenderBonusAttribute (AttributeName attributeName, int val) { attribute = attributeName; value = val; } }