patterncsharpMinor

Dynamic Storing Objects from XML in RPG

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

objectsrpgxmldynamicfromstoring

Problem

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 genders. Not sure what is preferred (or best).

An XML Example can be seen further down.

`///
/// Gender manager.
/// Access length by GenderManager.Length
/// Access gender by index GenderManager.gender[int]
///
/// Left to do: Singleton
///
/// Author: Emz
/// Date: 2014-01-21
///

using UnityEngine;
using System.Collections;
using System.Collections.Generic;
using System.Xml;
using System;

public class GenderManager : MonoBehaviour {

private static List genders;
private static int length;

// Use this for initialization
public GenderManager () {
genders = new List ();
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.S

Solution

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

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.

Using static fields in GenderManager and setting them via non-static constructor is a mess.

Length is not descriptive. What is length? If it is the length of genders, then why dont you expose genders.Count instead?

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.

I think some light weight data base will do a better job in storing game mechanics then xml files.

Context

StackExchange Code Review Q#39718, answer score: 4

Revisions (0)

No revisions yet.