0
\$\begingroup\$

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
        };
    }
}
\$\endgroup\$
5
  • \$\begingroup\$ The site standard is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$
    – BCdotWEB
    Dec 29, 2020 at 10:57
  • \$\begingroup\$ Are you sure the code works? There seems to be a mismatch between the return type and what is actually returned. What is the relationship between Template and TemplateDTO? \$\endgroup\$ Dec 29, 2020 at 13:24
  • \$\begingroup\$ @MrBalanikas Yep the code works as intended. Apologies, forgot to change that part. It is supposed to be TemplateMap not DTO. Will edit. \$\endgroup\$
    – Usman Khan
    Dec 29, 2020 at 13:32
  • \$\begingroup\$ XML|MPAN={}|IsCopy={} what is XML ? is it a valid XmlDocument? and what would {} means ? array ? could you give us a working sample of the ParameterName I need to know the full data structure that is being processed. \$\endgroup\$
    – iSR5
    Dec 30, 2020 at 12:07
  • \$\begingroup\$ @iSR5 Parameter name is a string that is retrieved as part of the template found in the database. XML|MPAN={}|IsCopy={} is how the string will come from the database. XML means that the parameter requires the standard xml for each template. If the parameter has ={} it means that it requires a dynamic value which I insert in my method. \$\endgroup\$
    – Usman Khan
    Dec 30, 2020 at 16:10

2 Answers 2

1
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks, in this case I only added that comment for this question as I didn't include the GetMetersInXml method. \$\endgroup\$
    – Usman Khan
    Dec 29, 2020 at 13:16
1
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ Apologies, there isn't any need to convert to a DTO object. The TemplateMap object is the only object needed. \$\endgroup\$
    – Usman Khan
    Dec 29, 2020 at 13:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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