XML document parser with XPath

Question

Here is my simple implementation on XML parser for XML which I received from a server response. Also I've tried to use XmlPathDocument but failed to get child elements for a node in XmlPathIterator.

 public OutboxResponse CheckSendResponseForOutboxPacket(Outbox outbox) { var responseContent = outbox.Outbox_content.FirstOrDefault().response_content; OutboxResponse outboxResponse = new OutboxResponse(); outboxResponse.Violations = new List<OutboxResponseViolation>(); XmlDocument responseDocument = new XmlDocument(); responseDocument.LoadXml(responseContent); var rootElement = responseDocument.DocumentElement; //XPath templates string responseResult = "r:body/r:result/text()"; string violationCode = "g:body/g:code/text()"; string violationLevel = "g:body/g:level/text()"; string violationName = "g:body/g:name/text()"; string violationDescription = "g:body/g:violations/g:violation"; string itemResultResult = "g:body/g:itemResults/g:itemResult/g:result/text()"; string itemResultViolations = "g:body/g:itemResults/g:itemResult/g:violations/g:violation"; XmlNamespaceManager nsmanager = new XmlNamespaceManager(responseDocument.NameTable); nsmanager.AddNamespace("g", "http://google.com"); switch (rootElement.SelectSingleNode(responseResult, nsmanager).Value) { case "success": outboxResponse.Result = "Packet received without errors"; foreach (XmlNode violation in rootElement.SelectNodes(itemResultViolations, nsmanager)) { outboxResponse.Violations.Add(new OutboxResponseViolation { Level = violation.SelectSingleNode("g:level/text()", nsmanager).Value, Name = violation.SelectSingleNode("g:name/text()", nsmanager).Value, Description = violation.SelectSingleNode("g:description/text()", nsmanager).Value }); } break; case "failure": outboxResponse.Result = "Errors was found in packet"; foreach (XmlNode violation in rootElement.SelectNodes(violationDescription, nsmanager)) { outboxResponse.Violations.Add(new OutboxResponseViolation { Level = violation.SelectSingleNode("r:level/text()", nsmanager).Value, Name = violation.SelectSingleNode("r:name/text()", nsmanager).Value, Description = violation.SelectSingleNode("r:description/text()", nsmanager).Value }); } break; } return outboxResponse; } 

Answer 1

Based on the naming guidelines properties shouldn't be named using some kind of Snake_Case casing. They should be named using PascalCase casing.

---

Without seeing OutboxResponse's implementation and its other usages, I would suggest initializing the Violations property in the class itself.

---

The loops inside the switch cases are very similiar. They only differ by the used "prefix" e and r. These should be extracted to a separate method to reduce code duplication.

---

You have a lot of vertical space wasted by using to many new lines. This reduces readability.

---

English isn't my first language so this could be wrong, but I guess "Errors was found in packet" should be "Errors were found in packet".

---

These variables

string violationCode = "g:body/g:code/text()"; string violationLevel = "g:body/g:level/text()"; string violationName = "g:body/g:name/text()"; string itemResultResult = "g:body/g:itemResults/g:itemResult/g:result/text()"; 

aren't used and should be removed.

---

You should always declare variables as near as possible to their usage.

---

By extracting the creation of the XMLDocument to a separate method you can reduce the size of the method which adds readability.

---

If you need a comment to describe for what a variable stands then this variable isn't properly named.

---

After applying the mentioned points the refactored method we would get

public OutboxResponse CheckSendResponseForOutboxPacket(Outbox outbox) { XmlDocument responseDocument = GetXMLDocument(outbox); XmlNamespaceManager nsmanager = new XmlNamespaceManager(responseDocument.NameTable); nsmanager.AddNamespace("g", "http://google.com"); var rootElement = responseDocument.DocumentElement; OutboxResponse outboxResponse = new OutboxResponse(); outboxResponse.Violations = new List<OutboxResponseViolation>(); string prefix; string xpath; string resultPath = "r:body/r:result/text()"; switch (rootElement.SelectSingleNode(resultPath, nsmanager).Value) { case "success": outboxResponse.Result = "Packet received without errors"; xpath = "g:body/g:itemResults/g:itemResult/g:violations/g:violation"; prefix = "g"; break; case "failure": outboxResponse.Result = "Errors were found in packet"; xpath = "g:body/g:violations/g:violation"; prefix = "r"; break; default: return outboxResponse; } XmlNodeList nodes = rootElement.SelectNodes(xpath, nsmanager); outboxResponse.Violations.AddRange(GetOutputViolations(nodes, nsmanager, prefix)); return outboxResponse; } private XmlDocument GetXMLDocument(Outbox outbox) { var content = outbox.Outbox_content.FirstOrDefault().response_content; XmlDocument document = new XmlDocument(); document.LoadXml(content); return document; } private IList<OutboxResponseViolation> GetOutputViolations(XmlNodeList nodes, XmlNamespaceManager nsmanager, string prefix) { IList<OutboxResponseViolation> violations = new List<OutboxResponseViolation>(); string levelPath = prefix + ":level/text()"; string namePath = prefix + ":name/text()"; string descriptionPath = prefix + ":description/text()"; foreach (XmlNode node in nodes) { violations.Add(new OutboxResponseViolation { Level = node.SelectSingleNode(levelPath, nsmanager).Value, Name = node.SelectSingleNode(namePath, nsmanager).Value, Description = node.SelectSingleNode(descriptionPath, nsmanager).Value }); } return violations; }