0
\$\begingroup\$

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;

    }
\$\endgroup\$
0

1 Answer 1

1
\$\begingroup\$

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;
}
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.