Nested foreach loop returning modified list

Question

In this method, I was tasked to take in a list of TemplateMap and attach dynamic parameters and in a certain case, add further templates to the returned list. This functionality exists within a report rendering service and at this stage, I am constructing the templates to send into the rendering function.

ParameterName in these templateMaps can have the following formats:

1. "XML|MPAN={}|IsCopy={}"
2. "XML|MPRN={}|IsCopy={}"
3. "XML"
4. "XML|IsCopy={}"

FYI the ParameterName is a string and the above is how I retrieve them from the database

private static IEnumerable<TemplateMap> TransformTemplateMaps(IEnumerable<TemplateMap> templateMaps, XmlDocument xml, bool isCopy = false)
{
    var transformedTemplates = new List<TemplateMap>();
    var bindingOrder = 1;

    foreach (var template in templateMaps)
    {
        var parameters = template.ParameterName.Split('|');
        var skipCurrentTemplate = false;

        foreach (var param in parameters)
        {
            var paramName = param.Split("=").First();

            switch (paramName.ToUpper())
            {
                case "MPRN":
                case "MPAN":
                    
                    // Returns a list of strings
                    var meters = GetMetersInXml(xml, paramName);

                    if ((meters ?? throw new InvalidOperationException()).Count == 1)
                    {
                        template.ParameterName = template.ParameterName.Replace($"{paramName}={{}}", $"{paramName}={meters.SingleOrDefault()}");
                    }
                    else
                    {
                        // Multiple meters per site requires one template each with different meter variables
                        foreach (var meter in meters)
                        {
                            // Initialise clone of template instance with different parameter values
                            var additionalTemplate = (TemplateMap) template.Clone();

                            additionalTemplate.ParameterName = additionalTemplate.ParameterName.Replace($"{paramName}={{}}", $"{paramName}={meter}");
                            additionalTemplate.ParameterName = additionalTemplate.ParameterName.Replace($"IsCopy={{}}", $"IsCopy={isCopy}");
                            
                            additionalTemplate.BindingOrder = bindingOrder;
                            bindingOrder++;

                            transformedTemplates.Add(additionalTemplate);
                        }

                        // Skips current template so that it doesn't add a duplicate template below
                        skipCurrentTemplate = true;
                    }
                    break;

                case "ISCOPY":
                    template.ParameterName = template.ParameterName.Replace($"{paramName}={{}}", $"{paramName}={isCopy}"); 
                    break;

                // add variables that require dynamic values here
            }
        }

        if (skipCurrentTemplate) 
            continue;

        template.BindingOrder = bindingOrder;
        bindingOrder++;
        transformedTemplates.Add(template);
    }

    return transformedTemplates;
}

In my code you can see that I have multiple nested foreach loops which I know to be bad design. I have found answers online that mention flattening out the nested loops into methods for better readability.

In my case however, I have certain properties i.e. bindingOrder and skipCurrentTemplate that need to be accessed from the inner loops so I'm not sure how to go about extracting these inner loops into methods as I'd have to return these other properties as well.

What way would you suggest I improve this code?

EDIT

Adding the TemplateMap object definition

public class TemplateMap : ICloneable
{
    public string ReportPath { get; set; }
    public string ParameterName { get; set; }
    public int BindingOrder { get; set; }
    public bool? IncludedInInvoice { get; set; }

    public object Clone()
    {
        return new TemplateMap
        {
            ReportPath = this.ReportPath,
            BindingOrder = this.BindingOrder,
            ParameterName = this.ParameterName,
            IncludedInInvoice = this.IncludedInInvoice
        };
    }
}

Answer 1

Some quick remarks:

• You should move the "convert template.ParameterName to values to iterate through" logic to a separate class and return a meaningful object. Now you twice have this code where you split a string and this adds noise. Especially since all you are interested in is the collection of paramNames.

• Considering that this is one private method which seems to call another private method, I'd argue that TransformTemplateMaps shouldn't be a method, but a class of its own. Then IEnumerable<TemplateMap> templateMaps, XmlDocument xml, bool isCopy = false could be class-level private fields, and you could easily split this method into smaller methods. This class could contain GetMetersInXml (or that could be a class of its own, if that method is long as well or if it needs to be used by other methods).

• Comments like // Returns a list of strings are pointless. Comments should explain why the code is the way it is (if necessary), not what the code does (this should be clear from the code itself).

• I realize naming is hard, but xml is way too vague. template doesn't seem to be the correct name, shouldn't it be templateMap?

• There is no reason to do meters.SingleOrDefault(). The if already checked that there is one entry in the collection, so .Single() is enough.

Answer 2

One common approach is to split the main problem into smaller subproblems. We can start from the innermost loop and move up nesting levels as we go. First subproblem is mainly about cloning the template, and that is subproblem1. Moving up to higher level, and the code seems to be about converting a template to a template DTO, and that is subproblem2.

Eventually the structure of the main problem will be something like:

main problem
    subproblem3
        subproblem2
            subproblem1

If you follow this, you will find out that there are lots of variables, state changes and parameters passed around, so you can also consider introducing a 'context' type, that groups the changeable state into a single data structure. That should make the code easier to understand.

Next thing to try is to make methods as pure as possible. Changing state within a method often makes code more difficult to grasp.

Finally, if you try to break down your code into multiple types and/or methods, then go for it wherever it makes sense, but only up to a certain point. After that point it is possible that you code becomes even more complicated than what it is now.

Those are just some of my hints. Try to experiment with different approaches until you reach something that makes sense, i.e. you should be able to quickly come back and understand the code even after you have been away for a year. Hopefully there already exists some unit tests to support you in the code refactoring.