current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite
1

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?

share|improve this question

2 Answers 2

up vote 4 down vote accepted
  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.
share|improve this answer
    
1. I try to use underscore as prefix when there are setters and getters for a variable. 2. a is attribute, v is value. 3. I will get to once I understand more about singletons. 4. Was part of my question, if that was better to do or not, I take it is better. 5. I was told so fairly recently, I used MSDN with a touch of stackoverflow to solve the XML part. 6. Yes, I will change that in the future. Right now, I want to make it as easy as possible for designers without having to craft tools for them. –  Emz Jan 21 '14 at 6:41
    
#3 I take it I should start look into singletons to clear that mess? #4 I should also return Genders.Length instead of having a length counter? In C++ it was faster to store the length and return it than use the innate of a vector if I can recall correctly. –  Emz Jan 21 '14 at 6:44
    
If you use a property instead of a method it's usually faster. In c++ the vector's size is a method, if I'm not mistaken. –  tinstaafl Jan 21 '14 at 7:01
    
@Emz, 1) variable has no setters or getters, only propeties do, and property name should start with a capital letter. 2) you do not need to explain it to me. You should not need to, thats the point. You should use attribute instead of a in the first place. 3) Singleton is a static instance of non-static class with private constructor. So you will have to tweak your class quite a bit. Also note, that singletons considered an anti-pattern by a lot of developers. In my opinion singletons are rarely needed and are often an excuse for laziness. –  Nikita Brizhak Jan 21 '14 at 9:05
    
@Emz, 4) yes, you should return Count. Even if there is a performance difference in C# (which i doubt), it is negligible. –  Nikita Brizhak Jan 21 '14 at 9:06
up vote 4 down vote

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;
    }
}
share|improve this answer
    
It is for the designers sake, so they don't have to remember 0 = Attack_Power (etc). It gets more complex when playing with Items. –  Emz Jan 21 '14 at 12:01
    
Reading my comment again, it doesn't much sense at all. Will add that change as well. It is good going over your code again now and then! –  Emz Jan 26 '14 at 8:48

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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