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;
}
1 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;
}