4
\$\begingroup\$

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

}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ I don't understand the Deserialize named methods. Typically one deserialise, when reading a file and creating an object representation of the content of that file. That does not seem to be the case here. List structure to represent UI elements are not optimal either - a tree structure is much more representative for that domain - you actually use the terms parent and child in your code. If you have a tree structure you can find any node in O(log n) time and you have O(1) access to the nodes children, grand children etc. \$\endgroup\$
    – ahybertz
    Oct 11 '14 at 10:04
  • \$\begingroup\$ @ahybertz they are segregated to deserialize different kinds of nodes instead deserializing in single method. The idea of Tree structure seems viable i will test with your suggestion. \$\endgroup\$ Oct 13 '14 at 8:29
  • \$\begingroup\$ @user1741928: Can you include example of the XML and/or code for WindowConfig? I had the same feeling as @ahybertz, because I did similar deserizalization for one of my projects (XML file like XAML or MXML). The XML can be first parsed to create some DOM (Document Object Model) with possible references which you handle in second pass (using Dictionary<String,Object>). \$\endgroup\$
    – user52292
    Oct 18 '14 at 12:48
3
\$\begingroup\$

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));
}
\$\endgroup\$
5
  • \$\begingroup\$ Thank you, unfortunately IEnumerable doesn't have Count member. Other wise its a good suggestion to change from List to IEnumerable. Can you think of any alternative to FindAll() extension method other than Linq 'Where'. When I profiled above methods the maximum time spent is on FindAll method. \$\endgroup\$ Sep 18 '14 at 13:38
  • \$\begingroup\$ @user1741928 It does have a Count() method. Where do you need a Count property ? \$\endgroup\$
    – Heslacher
    Sep 18 '14 at 13:39
  • \$\begingroup\$ In SortList() method the whole refactoring was based on Count () method. By the way nice assumption about VARIABLECONFIG its actually a string. \$\endgroup\$ Sep 18 '14 at 13:55
  • \$\begingroup\$ You need to add a using for Linq the you also have access to the Count() method which is used at the very top int listParamCount = listParam.Count(); \$\endgroup\$
    – Heslacher
    Sep 18 '14 at 13:57
  • \$\begingroup\$ Yes I should have added using statement. \$\endgroup\$ Sep 18 '14 at 13:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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