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?

• ChrisWue: 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.
• Berend: Could you perhaps provide a sample XML file? Judging by your code I suspect that it may be easier to use an XPath expression.
• Phylogenesis: @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.

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

• Abb: 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?
• Mark Shevchenko: @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: @AbhishekPandey And I updated my answer.
• Philip Kendall: @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.

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.

• svick: That's a really weird way to format a LINQ query. Why is a variable and its type on separate lines?