I want to convert these loops into simple loop. I want the code to look neat and short. I saw answers related to LINQ but couldn't make out from that. Are there any other possible ways for this snippet?

XmlDocument manifestXmlFile = new XmlDocument();
manifestXmlFile.Load(manifestFileName);

foreach (XmlNode rules in manifestXmlFile.DocumentElement.ChildNodes)
{
    foreach (XmlNode ruleNode in rules)
    {
        foreach (XmlNode childNodeAttributes in ruleNode)
        {
            foreach (XmlNode childNodeAttrib in childNodeAttributes.ChildNodes)
            {
                XmlElement ruleElement = (XmlElement)ruleNode;
                foreach (XmlNode childNodeConditions in childNodeAttrib.ChildNodes)
                {
                    foreach (XmlNode childNodeCond in childNodeConditions.ChildNodes)
                    {
                        if (childNodeCond.Name.ToUpper() == "CONDITION")
                        {
                            if (childNodeCond.Attributes["type"].Value.ToUpper() == "HEALTHY")
                            {
                                string ruleId = ruleElement.Attributes["ruleid"].Value;
                                string attributeName = childNodeAttrib.Attributes["name"].Value;
                                string attributeType = childNodeAttrib.Attributes["type"].Value;
                                string condTypeValue = childNodeCond.Attributes["type"].Value;
                                string operatorValue = childNodeCond.Attributes["operator"].Value;
                                string healthyConditionValue = childNodeCond.FirstChild.InnerText;

                                var guid = new Guid(ruleId);

                                //Conversion of enum types
                                PsmsOperator psmsOperator = (PsmsOperator)Enum.Parse(typeof(PsmsOperator), operatorValue, true);
                                TypeCode psmsAttributeType = (TypeCode)Enum.Parse(typeof(TypeCode), attributeType, true);

                                Rule rule = new Rule(guid, attributeName, healthyConditionValue, psmsOperator);

                                Rule(attributes, guid);
                           }
                        }
                    }
                }
            }
        }
    }
}

Is there any other better way than this or LINQ?

  • 1
    I haven't tried it but you should be able to use LINQ to XML and simply get all CONDITION nodes, something along these lines XDocument.Load(new StreamReader(manifestFileName)).Descendants("Condition") this would turn your 7 nested loops int o a single line. Might not work with the data you have but could be worth a shot. – ChrisWue Sep 22 '15 at 8:46
  • 3
    Could you perhaps provide a sample XML file? Judging by your code I suspect that it may be easier to use an XPath expression. – Berend Sep 22 '15 at 14:11
  • 1
    @Berend I agree. There's a few answers trying to replicate the exact loops presented, but this can surely be replaced by a single XPath query and looping over the results. – Phylogenesis Sep 22 '15 at 15:31
up vote 11 down vote accepted

I think you can extract small methods to simplify this code. Also you can try XPath to get specified sub-nodes and attributes.

And, yes, you can simplify this code with LINQ:

var ruleList = from rules in manifestXmlFile.DocumentElement.ChildNodes.Cast<XmlNode>()
               from ruleNode in rules
               from childNodeAttributes in ruleNode
               from childNodeAttrib in childNodeAttributes.ChildNodes.Cast<XmlNode>()
               from childNodeConditions in childNodeAttrib.ChildNodes.Cast<XmlNode>()
               from childNodeCond in childNodeConditions.ChildNodes.Cast<XmlNode>()
               where childNodeCond.Name.ToUpper() == "CONDITION"
               where childNodeCond.Attributes["type"].Value.ToUpper() == "HEALTHY"
               select new
               {
                   Id = new Guid(ruleElement.Attributes["ruleid"].Value),
                   AttributeName = childNodeAttrib.Attributes["name"].Value,
                   AttributeType = childNodeAttrib.Attributes["type"].Value,
                   CondTypeValud = condTypeValue = childNodeCond.Attributes["type"].Value,
                   OperatorValue = childNodeCond.Attributes["operator"].Value,
                   HealthyConditionValue = childNodeCond.FirstChild.InnerText,
               };

foreach (var r in ruleList)
{
    var psmsOperator = (PsmsOperator)Enum.Parse(typeof(PsmsOperator), r.OperatorValue, true);
    var psmsAttributeType = (TypeCode)Enum.Parse(typeof(TypeCode), r.AttributeType, true);

    var rule = new Rule(r.Id, r.AttributeName, r.HealthyConditionValue, psmsOperator);

    Rule(attributes, r.Id);
}
  • I get an error at line : from rules in manifestXmlFile.DocumentElement.ChildNodes for manifestXmlFile.DocumentElement.ChildNodes as could not find an implementation for the query pattern of the source type System.Xml.XmlNodeList and 'Select Many' not found.. Any idea why we get this? – Abb Sep 22 '15 at 10:16
  • @AbhishekPandey Well, I checked and found that XmlNodeList implements IEnumerable, but not IEnumerable<XmlNode>. So you can use the case manifestXmlFile.DocumentElement.ChildNodes.Cast<XmlNode>(). – Mark Shevchenko Sep 22 '15 at 11:12
  • @AbhishekPandey And I updated my answer. – Mark Shevchenko Sep 22 '15 at 11:13
  • @MarkShevchenko For what it's worth, that's because XmlNodeList has been around since the .NET 1.x days when generics where just a twinkle in Microsoft's eye. – Philip Kendall Sep 22 '15 at 12:38

Why do you define XmlElement ruleElement = (XmlElement)ruleNode; inside foreach (XmlNode childNodeAttrib in childNodeAttributes.ChildNodes) when you could define it two levels higher? Moreover, the same is true for string ruleId = ruleElement.Attributes["ruleid"].Value;.

Same for string attributeName = childNodeAttrib.Attributes["name"].Value; and string attributeType = childNodeAttrib.Attributes["type"].Value;: these too could be defined at a much higher level.


Your naming is all over the place: rules contain ruleNode, ruleNode contains childNodeAttributes, etc. This makes it really hard to see the structure. The ubiquitous use of the childNode prefix is an extra hindrance.

Also avoid unnecessary abbreviations like childNodeCond.


Don't do this: if (childNodeCond.Name.ToUpper() == "CONDITION") and if (childNodeCond.Attributes["type"].Value.ToUpper() == "HEALTHY"). If you need to compare strings while ignoring case, do it properly:

if(string.Equals(childNodeCond.Name, "CONDITION", StringComparison.OrdinalIgnoreCase))

You're not considering various possible problems:

  • what if manifestXmlFile.DocumentElement is null?
  • what if childNodeCond.Attributes is null?
  • what if childNodeAttrib.Attributes is null?
  • what if these XmlAttributeCollections do not contain the keys for which you're retrieving values?

You have a method called Rule(): Rule(attributes, guid);? That's not a good name, and moreover it is a very confusing name in this context.


I find code like foreach (XmlNode ruleNode in rules), where rules is an XmlNode, to be very confusing. Sure it compiles, but it is really confusing. I'm assuming this actually enumerates the ChildNodes collection, so why then don't you explicitly say so?

Naming is also an issue here: rules suggest an IEnumerable<T> or an ICollection<T> and instead it is... an XmlNode? Same for childNodeAttributes and childNodeConditions.


The big problem to solve is of course those six levels of foreach. I would be very tempted to convert this method into a class of its own and have class-level variables -- ruleId, attributeName etc. -- and convert each foreach level into a properly named method.


Is this code complete? psmsAttributeType and rule seem to be unused, and attributes isn't defined anywhere.

You might want to think about reducing the nesting in your IF statments

        if(childNodeCond.Name.ToUpper() != "CONDITION")
        {
            continue;
        }
        if(childNodeCond.Attributes["type"].Value.ToUpper() != "HEALTHY")
        {
            continue;
        }

You are doing alot of looping and translating it into linq isn't the smartest idea in the world as it's a difficult to understand mess.

            foreach(var guid in from XmlNode 
                        rules in manifestXmlFile.DocumentElement.ChildNodes from XmlNode 
                        ruleNode in rules from XmlNode
                        childNodeAttributes in ruleNode from XmlNode 
                        childNodeAttrib in childNodeAttributes.ChildNodes 
                        let ruleElement = (XmlElement)ruleNode from XmlNode 
                        childNodeConditions in childNodeAttrib.ChildNodes from XmlNode 
                        childNodeCond in childNodeConditions.ChildNodes 
                        where childNodeCond.Name.ToUpper() == "CONDITION" 
                        where childNodeCond.Attributes["type"].Value.ToUpper() == "HEALTHY" 
                        let ruleId = ruleElement.Attributes["ruleid"].Value 
                        let attributeName = childNodeAttrib.Attributes["name"].Value 
                        let attributeType = childNodeAttrib.Attributes["type"].Value 
                        let operatorValue = childNodeCond.Attributes["operator"].Value 
                        let healthyConditionValue = childNodeCond.FirstChild.InnerText 
                        let guid = new Guid(ruleId) 
                        let psmsOperator = (PsmsOperator)Enum.Parse(typeof(PsmsOperator), operatorValue, true) 
                        let psmsAttributeType = (TypeCode)Enum.Parse(typeof(TypeCode), attributeType, true) 
                        let rule = new Rule(guid, attributeName, healthyConditionValue, psmsOperator) select guid)
            {
                Rule(attributes, guid);
            }

Please note that you aren't using the condTypeValue variable.

  • That's a really weird way to format a LINQ query. Why is a variable and its type on separate lines? – svick Sep 23 '15 at 6:55

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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