xml is XML is xml is XML

Question

I had to write some code for an interview, to print out the name attribute of each node. I started off going in the wrong direction, so I didn't get to finish. I wanted to finish writing the code, so I created my console app similar to what they had a button event doing. Please let me know what you think of the code and what I can do better.

class XMLRecursionReader
{
 private StringBuilder _outputString = new StringBuilder();
 private XmlNode _root;
 public XMLRecursionReader(XmlDocument xDoc)
 {
 _root = xDoc.ChildNodes[1]; 
 }
 public string ReturnNameAsString (XmlNode node)
 {
 return node.Attributes["name"].InnerXml.ToString();
 }
 public void buildString (XmlNode node)
 {
 _outputString.AppendLine(ReturnNameAsString(node));
 if (node.HasChildNodes)
 {
 foreach (XmlNode childNode in node.ChildNodes)
 {
 buildString(childNode);
 }
 }
 }
 public void PrintOutput ()
 {
 buildString(_root);
 Console.WriteLine(_outputString.ToString());
 Console.ReadLine();
 }
}

similar sample to what I was coding against. but the code needed to be generic and able to go as deep as necessary depending on the document that was fed in, but the structure is always going to be similar to this.

<?xml version="1.0" encoding="UTF-8"?>
<report name="ReportName">
 <agency name="agency1">
 <office name="office1"></office>
 <office name="office2"></office>
 <office name="office3"></office>
 </agency>
 <agency name="agency2">
 <office name="office1">
 <agent name="agent Amy"></agent>
 <address name="address line"></address>
 </office>
 <office name="office2"></office>
 <office name="office3"></office>
 </agency>
 <agency name="agency3">
 <office name="office1">
 <agent name="agent Bettie">
 <subagent name="sub-agent bob">
 <phone name="456-789-1230"></phone>
 </subagent>
 <subagent name="sub-agent billy"></subagent>
 </agent>
 <address name="address line">
 <faxnumber name="1234567890"></faxnumber>
 </address> 
 </office>
 <office name="office2"></office>
 <office name="office3"></office>
 </agency>
</report>

The Results:

Output of xml is XML is xml is XML

Answer 1

One should also add that:

• Both the ReturnNameAsString and the buildString methods could be private as they are not very useful as public APIs.
• Additionally ReturnNameAsString can also be static because it doesn't use any private instance data.

• It would be easier to do it with a XDocument because then you can cheat which makes it virtually a one-liner:

  var names = 
   XDocument
   .Parse(xml)
   .Root
   .Descendants()
   .Select(x => x.Attribute("name").Value)
   .ToList();
  

• t3chb0t: @Malachi it does work, I tested it in LINQPad myself before posting. You must be careful with the xml, it must not contain any whitespaces before the <?xml see this gist
• Malachi: I first had to use .Load instead of .Parse and then it gives me the Root error, and I have to use a foreach to get the items out of it anyway
• Johnbot: To match the implementation in the question it should be DescendantsAndSelf() so the name of the root is included or just remove the .Root instead. But it's hard to tell with no expected output.
• lukkea: "virtually" a one-liner?! It's a one-liner dude.
• t3chb0t: @lukkea haha ;-) yeah, I just prettfied it and un-one-lined it.

Answer 2

Naming

There are .NET Naming Guidelines which state that methods should be named using PascalCase casing. You haven't done this for buildString().

The code

• The InnerXml property returns a string hence there is no need to call ToString() on the property.
• Because all methods are public you should do proper parameter validation. At least you should check if the passed parameter is null and if yes throw an ArgumentNullException.
• The PrintOutput () method does way too much. It builds the output, writes it to the console and reads a line from the console.
• Because _root won't change you should make it readonly.
• Calling PrintOutput twice results in doubling the printed value because you never reset/clear the StringBuilder.
• The method name of ReturnNameAsString is too much. Because the method isn't void it is clear that it returns something. From looking at the returned type (string) it is clear what the method returns. Just call it GetNodeName instead.
• The class name XMLRecursionReader exposes too much information about the implementation. A user of your code won't need to know that you solve the task by using recursion.
• Both methods buildString and ReturnNameAsString should be private.
• The if (node.HasChildNodes) in the buildString method is superflous because you are using a foreach to iterate over the ChildNodes. If there aren't any ChildNodes the foreach won't iterate.
• The constructor assumes that the passed XmlDocument containes ChildNodes which may not be the case.

• wizzwizz4: Another rationale for the naming - if you later change it so that it doesn't use recursion... then what?