Recursive reading of a UI configuration file

Question

In our project (Windows app, Winforms), the UI is designed based on XML configuration file. I have a program to read the UI configuration file and Parse the configuration and design the UI.

The configuration file is designed for a complex UI. The UI has Frames, and Frames can have Tabs, Tabs->Tabs-> finally one control to display the content.

Following are two methods which are heavily used recursively. Their hit count is roughly 2000 or more times.

1. DeserializeChildren
2. DeserializeFrames

Inside these methods we have plain old nested, multiple for loops. I try to refactor these methods but I couldn't find any alternative way to do the program other than what it is currently.

private void DeserializeChildren(WindowConfig config, string parent)
{
    if (config.Frame.ChildControls == null)
    {
        config.Frame.ChildControls = DeserializeFrames(parent);
    }

    if (config.Commands == null)
    {
        config.Commands = DeserializeCommands(parent);
    }

    List<SerializerContainer> scList = list.FindAll(sc => (sc.Parent == parent) &&
                                                          (sc.Processed == false) &&
                                                          (sc.ConfigType == WINDOWCONFIG));
    List<WindowConfig> wcList = new List<WindowConfig>();

    scList = ListSort(scList);

    foreach (SerializerContainer sc in scList)
    {
        string nextParent = sc.Name;
        WindowConfig windowConfig; // = new WindowConfig();
        windowConfig = DeserializeIt<WindowConfig>(sc.Config);
        sc.Processed = true;

        wcList.Add(windowConfig);
        DeserializeChildren(windowConfig, parent + nextParent);
    }
    if (wcList.Count > 0)
    {
        config.ChildWindows = wcList.ToArray();
    }

    return;
}

The above code has a call to DeserializeFrames, which is a recursive method.

private ControlItemConfig[] DeserializeFrames(string parent)
{
    List<SerializerContainer> scList = list.FindAll(sc => (sc.Parent == parent) &&
                                              (sc.Processed == false) &&
                                              (sc.ConfigType == CONTROLITEMCONFIG));
    List<ControlItemConfig> cicList = new List<ControlItemConfig>();

    scList = ListSort(scList);

    foreach (SerializerContainer sc in scList)
    {
        ControlItemConfig itemConfig; // = new ControlItemConfig();
        itemConfig = DeserializeIt<ControlItemConfig>(sc.Config);

        sc.Processed = true;

        cicList.Add(itemConfig);

        if ((itemConfig.Type == ControlType.Frame))
        {
            var frameConfig = (FrameConfig)itemConfig.Instance;
            if (frameConfig.ChildControls == null)
            {
                frameConfig.ChildControls = DeserializeFrames(sc.Parent + sc.Name);
            }
        }
    }
    return cicList.ToArray();
}

Other smaller methods involved are:

private CommandConfig[] DeserializeCommands(string parent)
{
    List<SerializerContainer> scList = list.FindAll(sc => (sc.Parent == parent) &&
                                              (sc.Processed == false) &&
                                              (sc.ConfigType == COMMANDCONFIG));
    List<CommandConfig> ccList = new List<CommandConfig>();

    scList = ListSort(scList);

    foreach (SerializerContainer sc in scList)
    {

        CommandConfig commandConfig; 
        commandConfig = DeserializeIt<CommandConfig>(sc.Config);

        sc.Processed = true;

        ccList.Add(commandConfig);

    }
    return ccList.ToArray();

}

private VariableConfig[] DeserializeSortedVariables(string parent)
{
    List<SerializerContainer> scList = list.FindAll(sc => (sc.Parent == parent) &&
                                              (sc.Processed == false) &&
                                              (sc.ConfigType == VARIABLECONFIG));
    List<VariableConfig> ccList = new List<VariableConfig>();

    scList = ListSort(scList);

    foreach (SerializerContainer sc in scList)
    {

        VariableConfig variableConfig;
        variableConfig = DeserializeIt<VariableConfig>(sc.Config);

        sc.Processed = true;

        ccList.Add(variableConfig);

    }
    return ccList.ToArray();

}



private List<SerializerContainer> ListSort(List<SerializerContainer> listParam)
{
    List<SerializerContainer> imList = listParam.FindAll(sc => (sc.Source == "XXX"));
    List<SerializerContainer> dmList = listParam.FindAll(sc => (sc.Source == "YYY"));

    imList.Sort();
    dmList.Sort();

    SerializerContainer[] scArray = new SerializerContainer[listParam.Count];

    //the goal is to use the absolute position value for XXX as set by 
    //each node's order value.  after processing the XXX, process the yyyy data,
    //plugging nodes into the array being built in the first open position found starting from
    //the front of the array
    //iterate through the instrument manager's configuration list from end to start
    int idx;
    for (int i = imList.Count - 1; i >= 0; i--)
    {
        //if the config node's order is greater than the number of elements in overall list 
        //set the index to the point to the last element in the array we are populating; 
        //otherwise set the index to the node's order value.
        if (imList[i].Order >= listParam.Count)
        {
            idx = listParam.Count - 1;
        }
        else
        {
            idx = imList[i].Order - 1;
        }

        //find a spot in the array that is not occupied and put the node there.
        while (scArray[idx] != null)
        {
            idx--;
        }
        if (idx >= 0)
        {
            scArray[idx] = imList[i];
        }
        else
        {
            throw new ArgumentOutOfRangeException(@"The index for the configuration list sort operation is less than zero.");
        }
    }

    idx = 0;
    //plug im nodes in wherever we find an open spot in the array
    foreach (SerializerContainer sc in dmList)
    {
        while (scArray[idx] != null)
        {
            idx++;
        }

        scArray[idx] = sc;
    }

    return new List<SerializerContainer>(scArray);

}

Answer 1

What you did good
Generally the naming of your methods with its parameters is good.
You are using braces { } also for single line if commands which is very good.

What you can do better
In every method (except ListSort()) you are calling list.FindAll() with different parameter for only the ConfigType, so it would be good start to extract this into a separate method.

You should use IEnumerable<> instead of List<> as the former will only gather the values if accessed. See https://stackoverflow.com/a/3628705/2655508

If you don't need the numbers of items in a List or of an IEnumerable but you need to check if it has any items, you should use the .Any() method as it performs faster.

For the FindAll condition, you should always check Booleans first, then Enums and last Strings for performance.

The only nitpicking I will do for naming is for the ListSort() method. Based on the naming convention a method name should be verbs or verb phrases, so ListSort() should be SortList().

Refactoring

Let us start by extracting the calls of list.FindAll() to some extension methods.

As I don't know what datatype e.g VARIABLECONFIG is , I have added an enum ConfType which can be also replaced by an String etc.

public enum ConfType
{
    WINDOWCONFIG, CONTROLITEMCONFIG, COMMANDCONFIG, VARIABLECONFIG
}

---

public static IEnumerable<SerializerContainer> AsFrames(this IEnumerable<SerializerContainer> list){
    return list.FilterBy(ConfType.CONTROLITEMCONFIG);
}
public static IEnumerable<SerializerContainer> AsVariables(this IEnumerable<SerializerContainer> list)
{
    return list.FilterBy(ConfType.VARIABLECONFIG);
}
public static IEnumerable<SerializerContainer> AsCommands(this IEnumerable<SerializerContainer> list)
{
    return list.FilterBy(ConfType.COMMANDCONFIG);
}
public static IEnumerable<SerializerContainer> AsWindows(this IEnumerable<SerializerContainer> list)
{
    return list.FilterBy(ConfType.WINDOWCONFIG);
}
private static IEnumerable<SerializerContainer> FilterBy(this IEnumerable<SerializerContainer> list, ConfType confType)
{
    return list.Where(i => i.ConfigType == confType);
}
public static IEnumerable<SerializerContainer> FilterBySource(this IEnumerable<SerializerContainer> list, String source)
{
    return list.Where(i => i.Source == source);
}
public static IEnumerable<SerializerContainer> FilterByProcessed(this IEnumerable<SerializerContainer> list, Boolean processed)
{
    return list.Where(i => i.Processed == processed);
}
public static IEnumerable<SerializerContainer> AsChildrenOf(this IEnumerable<SerializerContainer> list,String parent)
{
    return list.Where(i => i.Parent == parent);
}

---

Next we should refactor
private List<SerializerContainer> ListSort(List<SerializerContainer> listParam)

As I couldn't figure out what it really does let us change the name , the type of listParam and also the return type to IEnumerable.

private IEnumerable<SerializerContainer> SortList(IEnumerable<SerializerContainer> listParam)
{

    if (!listParam.Any()) { return Enumerable.Empty<SerializerContainer>(); }

    int listParamCount = listParam.Count();
    SerializerContainer[] scArray = new SerializerContainer[listParamCount];

    List<SerializerContainer> imList = listParam.FilterBySource("XXX").ToList();
    List<SerializerContainer> dmList = listParam.FilterBySource("YYY").ToList();

    imList.Sort();
    dmList.Sort();

    int idx;
    for (int i = imList.Count - 1; i >= 0; i--)
    {
        if (imList[i].Order >= listParamCount)
        {
            idx = listParamCount - 1;
        }
        else
        {
            idx = imList[i].Order - 1;
        }

        while (scArray[idx] != null)
        {
            idx--;
        }

        if (idx >= 0)
        {
            scArray[idx] = imList[i];
        }
        else
        {
            throw new ArgumentOutOfRangeException(@"The index for the configuration list sort operation is less than zero.");
        }
    }

    idx = 0;

    foreach (SerializerContainer sc in dmList)
    {
        while (scArray[idx] != null)
        {
            idx++;
        }

        scArray[idx] = sc;
    }

    return scArray.AsEnumerable();
}  

Maybe it is possible to call this method once with all items of the original list. Your application would gain a lot regarding performance.

---

Now we can start to refactor the smaller methods which aren't called recursively.

private List<CommandConfig> DeserializeCommands(IEnumerable<SerializerContainer> nonProcessedChildren)
{
    List<CommandConfig> commandList = new List<CommandConfig>();

    if (!nonProcessedChildren.Any()) { return commandList; }

    foreach (SerializerContainer sc in SortList(nonProcessedChildren))
    {
        commandList.Add(DeserializeIt<CommandConfig>(sc.Config));
        sc.Processed = true;
    }

    return commandList;
}  

---

private List<VariableConfig> DeserializeSortedVariables(IEnumerable<SerializerContainer> nonProcessedChildren)
{

    List<VariableConfig> variableList = new List<VariableConfig>();

    if (!nonProcessedChildren.Any()) { return variableList; }

    foreach (SerializerContainer sc in SortList(nonProcessedChildren))
    {
        variableList.Add(DeserializeIt<VariableConfig>(sc.Config));
        sc.Processed = true;
    }

    return variableList;
}  

Now let us refactor private ControlItemConfig[] DeserializeFrames(string parent)

private List<ControlItemConfig> DeserializeFrames(string parent, IEnumerable<SerializerContainer> nonProcessedFrames)
{
    List<ControlItemConfig> cicList = new List<ControlItemConfig>();

    IEnumerable<SerializerContainer> childFrames = nonProcessedFrames.AsChildrenOf(parent);

    if (!childFrames.Any()) { return cicList; }

    foreach (SerializerContainer sc in SortList(childFrames))
    {
        ControlItemConfig itemConfig = DeserializeIt<ControlItemConfig>(sc.Config);

        sc.Processed = true;

        cicList.Add(itemConfig);

        if ((itemConfig.Type == ControlType.Frame))
        {
            var frameConfig = (FrameConfig)itemConfig.Instance;
            if (frameConfig.ChildControls == null)
            {
                frameConfig.ChildControls = DeserializeFrames(sc.Parent + sc.Name, nonProcessedFrames).ToArray();
            }
        }
    }

    return cicList;
}  

As you see, this method now takes a second parameter which is called nonProcessedFrames which is basically the original list, with FilterByProcessed(false) in combination with AsFrames() like shown in the refactored DeserializeChildren() method below

private void DeserializeChildren(WindowConfig config, string parent, IEnumerable<SerializerContainer> nonProcessedList)
{

    IEnumerable<SerializerContainer> nonProcessedParents = nonProcessedList.AsChildrenOf(parent);
    IEnumerable<SerializerContainer> nonProcessedWindowsParents = nonProcessedParents.AsWindows();

    if (!nonProcessedList.Any() 
        || !nonProcessedParents.Any() 
        || !nonProcessedWindowsParents.Any()) { return; }

    if (config.Frame.ChildControls == null)
    {
        IEnumerable<SerializerContainer> nonProcessedFrames = nonProcessedList.AsFrames();
        config.Frame.ChildControls = DeserializeFrames(parent, nonProcessedFrames).ToArray();
    }

    if (config.Commands == null)
    {
        IEnumerable<SerializerContainer> nonProcessedCommands = nonProcessedParents.AsCommands();
        config.Commands = DeserializeCommands(nonProcessedCommands).ToArray();
    }

    List<WindowConfig> wcList = new List<WindowConfig>();

    foreach (SerializerContainer sc in SortList(nonProcessedWindowsParents))
    {
        WindowConfig windowConfig = DeserializeIt<WindowConfig>(sc.Config);
        sc.Processed = true;
        wcList.Add(windowConfig);

        DeserializeChildren(windowConfig, parent + sc.Name, nonProcessedList);
    }

    if (nonProcessedWindowsParents.Any())
    {
        config.ChildWindows = wcList.ToArray();
    }
}

As I don't know how you call the DeserializeChildren() method the first time, let us overload it, so you can call it like you did.

private void DeserializeChildren(WindowConfig config, string parent)
{
    DeserializeChildren(config, parent, list.FilterByProcessed(false));
}