Convert Foreach Loop into Linq in C#

Question

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?

Answer 1

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

Answer 2

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.

Answer 3

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.