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