Parse through an XDocument efficiently and match values to matching object

Question

Minimum viable test program:

class Program
{
 private static List<Param> paramList;
 private class Param
 {
 public string xmlName;
 public double scalingFactor;
 public double Value { get; set; }
 public Param( string xmlName, double scalingFactor)
 {
 this.xmlName = xmlName;
 this.scalingFactor = scalingFactor;
 }
 }
 private static void UpdateParams(XDocument xDoc)
 {
 IEnumerable<Param> filteredParamList;
 foreach (XElement xElement in xDoc.Root.Elements())
 {
 filteredParamList = paramList.Where(param => param.xmlName == xElement.Name.LocalName);
 if (filteredParamList != null && filteredParamList.Any())
 {
 filteredParamList.Single().Value = double.Parse(xElement.Value) * filteredParamList.Single().scalingFactor;
 }
 }
 }
 static void Main(string[] args)
 {
 paramList = new List<Param>
 {
 new Param("paramA",1),
 new Param("paramB",.5),
 new Param("paramC",.01)
 };
 /* For testing, this XDocument is hard coded here. */
 UpdateParams(XDocument.Parse("<root><paramA>121</paramA><paramB>100</paramB><paramC>197</paramC></root>"));
 foreach(Param param in paramList)
 {
 Console.WriteLine("Name: {0} Value: {1}", param.xmlName, param.Value);
 }
 Console.Read();
 }
}

Let's focus on UpdateParams. So I have an XDoc that has a root and values. Looks like this:

<root>
 <paramA>121</paramA>
 <paramB>100</paramB>
 <paramC>105</paramC>
 ...
</root>

I have a List of my param objects with an xmlName field which matches my XML key's. The xmlName of each Param object in my list will be unique; I'm manually creating every Param class in the main class's constructor (or in this test program, the main function). For context, the param objects will eventually have UI/WPF bindings from Value to textboxes or other UI elements as needed. For each key in the XML, i want to find the matching param object from my list, and fill in the Value field. If there's something weird in my XDocument that doesn't match my object list, I want to ignore it (and not throw an exception).

Is there a simpler/better way to write the parsing? Seems a little verbose, and i don't like that I have that extra IEnumerable filteredParamList just to work around null checking.

• t3chb0t: Using .Single() is usually very dangours. Is it guaranteed that there will be only single match? I'd be great if you could post the rest of the code too. There are some undefined variables.
• yhyrcanus: Yep, I can guarantee that xmlName will be unique through the entire paramList. If .Any() returns true, .Single() will pass.
• pacmaninbw: As @t3chb0t indicated it would be better if you could post more of the code to provide context.

Answer 1

I'm going to revew only this part as I think the rest of it is just for demonstration purposes.

private static void UpdateParams(XDocument xDoc)
{
 IEnumerable<Param> filteredParamList;
 foreach (XElement xElement in xDoc.Root.Elements())
 {
 filteredParamList = paramList.Where(param => param.xmlName == xElement.Name.LocalName);
 if (filteredParamList != null && filteredParamList.Any())
 {
 filteredParamList.Single().Value = double.Parse(xElement.Value) * filteredParamList.Single().scalingFactor;
 }
 }
}

• Try to avoid writing methods like this one, where one of the parameters is some static field. This makes it very difficult to test because of the external dependency. If possible you should pass the parameter as anoter argument. I had a hard time understanding your code.

• Try to also avoid modifying methods parameters like you do here with the paramList. It's usually much better to return a new result. Such methods are called pure methods and are much easier to test and to maintain. Modifying global objects can make debugging very painful.

• Use the smallest possible type for parameters (or the most general/abstract). This means, your method doesn't need the XDocument, it can do its job just fine with an IEnumerable<XElement>.

• Choose more meaningful method names. UpdateParams is too general and doesn't say anything about what is going to be updated.

• You neither need the foreach, nor Where, nor Any, nor Single, nor the null check. All this can be replaced by a much nicer join.

---

This is how it could look like after refactoring:

private static IEnumerable<Param> ScaleParameterValues
(
 IEnumerable<XElement> elements, 
 IEnumerable<Param> parameters
)
{
 return
 from xe in elements
 join p in parameters on xe.Name.LocalName equals p.xmlName
 select new Param(p.xmlName, p.scalingFactor) 
 { 
 Value = p.scalingFactor * double.Parse(xe.Value) 
 };
}

• yhyrcanus: That makes perfect sense! I'm obviously new to C# and Linq, it didn't click to me that linq gives me practically everything from sql (including joins).