Saturday, March 21, 2009

Refactored code...which is better?

this morning, i was editing some code, and Resharper warned me about a possible System.NullReferenceException. i'd been bitten before by this in the same code, but that was because i didn't understand how XmlReader worked then. here is the original code:


MessageDatabase messageDatabase = null;
MessageTable messageTable = null;

while (configReader.Read())
{
  if (configReader.NodeType == XmlNodeType.Element)
  {
    // ...
    if (configReader.Name == "messagedatabase")
    {
      messageDatabase = new MessageDatabase();
      messageDatabase.Name = 
        configReader.GetAttribute("name");
      MessageDatabases.Add(messageDatabase);
    }
    if (configReader.Name == "messagetable")
    {
       messageTable = new MessageTable();
       messageTable.Name =
        configReader.GetAttribute("name");
       messageDatabase.MessageTables.Add(messageTable);
    }
    if (configReader.Name == "identityfield")
      messageTable.IdentityFieldName =
        configReader.ReadElementString();
    // ...

as you can probably tell, it's supposed to parse an XML file and convert it into a bunch of objects. nothing wrong with that. it takes into account the structure of the XML and makes an assumption based on that, namely, the <messagedatabase> element is always processed before the <messagetable> element, so messageDatabase will be set to an existing MessageDatabase object before any MessageTable instance is created.

a more explicit version of the code is shown below:


while (configReader.Read())
{
  if (configReader.NodeType != XmlNodeType.Element) continue;
  // ...
  if (configReader.Name == "messagedatabase")
  {
    var messageDatabase = new MessageDatabase
    {
      Name = configReader.GetAttribute("name")
    };
    MessageDatabases.Add(messageDatabase);
  }
  if (configReader.Name == "messagetable")
  {
    var index = MessageDatabases.Count - 1;
    var messageTable = new MessageTable
    {
      Name = configReader.GetAttribute("name")
    };
    var currentDb = MessageDatabases[index];
    currentDb.MessageTables.Add(messageTable);
  }
  if (configReader.Name == "identitfyfield")
  {
    var dbIndex = MessageDatabases.Count - 1;
    var currentDb = MessageDatabases[dbIndex];
    var tblIndex = currentDb.MessageTables.Count - 1;
    var currTbl = currentDb.MessageTables[tblIndex];
    currTbl.IdentityField =
      configReader.ReadElementString();
  }

while this is more explicit, the code is simply more. for each property of the MessageTable, i must get the index of the last database and the last table in it, and then set the values accordingly. that plain sucks.

there was one good thing about writing this post: i finally thought about where i could optimize the loop that advances configReader, and added continue statements where they wouldn't cause exceptions. :D

No comments: