Building tree structure based on flat objects

Question

Description

A List<UnrelatedObects> is returned by a 3rd party webservice, which will then be transformed to a List<ArchiveDefinition>. These ArchiveDefinition objects are connected by Parent'.ArchiveNodeId == 'Child'.ParentId.

The root objects TypeOfArchive property will always have the value ArchiveType.Archive.

The goal of the given class below is to build a List<ArchiveTreeNode> of these flat object list to fill a treeview control.

The class in question

public class ArchiveBuilder
{
    public static List<ArchiveTreeEntry> Build(List<ArchiveDefinition> entries)
    {
        List<ArchiveTreeEntry> rootArchiveTreeEntries = new List<ArchiveTreeEntry>();

        if (entries != null && entries.Count > 0)
        {
            List<ArchiveDefinition> rootEntries = GetRootEntries(entries);

            foreach (ArchiveDefinition definition in rootEntries)
            {
                rootArchiveTreeEntries.Add(new ArchiveTreeEntry(definition));
                entries.Remove(definition);
            }

            foreach (ArchiveTreeEntry parent in rootArchiveTreeEntries)
            {
                FillChildren(parent, entries);
            }
        }
        return rootArchiveTreeEntries;
    }

    private static void FillChildren(ArchiveTreeEntry parent,
                                     List<ArchiveDefinition> entries)
    {
        if (entries.Count > 0)
        {
            List<ArchiveDefinition> children = GetChildren(entries, parent.Id);

            if (children.Count > 0)
            {
                RemoveChildren(entries, parent.Id);

                foreach (ArchiveDefinition child in children)
                {
                    ArchiveTreeEntry treeEntryChild = new ArchiveTreeEntry(child);
                    parent.AddChild(treeEntryChild);
                    FillChildren(treeEntryChild, entries);
                }
            }
        }
    }

    private static List<ArchiveDefinition> GetRootEntries(List<ArchiveDefinition> entries)
    {
        return entries.FindAll(e => e.TypeOfArchive == ArchiveType.Archive);
    }

    private static List<ArchiveDefinition> GetChildren(List<ArchiveDefinition> entries, string parentID)
    {
        return entries.FindAll(e => e.ParentId == parentID);
    }

    private static void RemoveChildren(List<ArchiveDefinition> entries, string parentID)
    {
        entries.RemoveAll(e => e.ParentId == parentID);
    }
}

Related classes and enums

public class ArchiveDefinition
{
    public string ArchiveNodeId { get; private set; }
    public string ParentId { get; private set; }
    public ArchiveType TypeOfArchive { get; private set; }

    public ArchiveDefinition (String parentId, String archiveNodeId,
                              ArchiveType type)
    {
        ParentId = parentId;
        TypeOfArchive = type;
        ArchiveNodeId = archiveNodeId;
    }
}

public enum ArchiveType
{
    Archive, ArchiveGroup, ArchiveEntry
}

public class ArchiveTreeEntry
{
    public ArchiveType ArchiveEntryType { get; private set; }
    public string Id { get; private set; }
    public ReadOnlyCollection<ArchiveTreeEntry> Children
    {
        get
        {
            return new ReadOnlyCollection<ArchiveTreeEntry>(mChildren);
        }
    }

    private List<ArchiveTreeEntry> mChildren = new List<ArchiveTreeEntry>();
    public ArchiveTreeEntry(ArchiveDefinition archiveDefinition)
    {
        Id = archiveDefinition.ArchiveNodeId;
        ArchiveEntryType = archiveDefinition.TypeOfArchive;
    }

    internal void AddChild(ArchiveTreeEntry child)
    {
        if (child != null)
        {
            mChildren.Add(child);
        }
    }
}

I would like to get a review for the ArchiveBuilder class. If you also want to review the related classes and enums, I won't mind.

Olorin71 · Accepted Answer · 2014-08-01 22:27:28Z

I think your implementation is pretty clear. Nonetheless I tried some changes, hoping my version is more readable.

I agree with Stuart's post. (using IEnumerable<> if possible, non static Build() function, ...).

In Addition I think you have broken this principles:

Single Responsibility --> Your FillChildren(...) function is looking for children and removing them from the original list and adding them in the parent's children collection. I mean that are three responsibilities.
Least Attonishment --> In a function called FillChildren I doesn't expect anything being removed.

I changed also many Names, but this may be a matter of taste.

Here is the changed code:

public class ArchiveBuilder
{
    public List<ArchiveTreeEntry> Build(List<ArchiveDefinition> availableArchiveDefinitions)
    {
        List<ArchiveTreeEntry> rootArchiveTreeEntries = null;

        if (availableArchiveDefinitions != null && availableArchiveDefinitions.Count > 0)
        {
            rootArchiveTreeEntries = CreateRootArchiveTreeEntries(availableArchiveDefinitions);
            availableArchiveDefinitions = RemoveRootArchiveDefinitions(availableArchiveDefinitions);
            foreach (var entry in rootArchiveTreeEntries)
            {
                HandleAvailableEntriesForGivenParent(availableArchiveDefinitions, entry);
            }
        }

        return rootArchiveTreeEntries;
    }

    private static void AssignChildrenToParent(ArchiveTreeEntry parent, 
        IEnumerable<ArchiveDefinition> children)
    {
        parent.AddChildRange(children.Select(x => new ArchiveTreeEntry(x)));
    }

    private static List<ArchiveTreeEntry> CreateRootArchiveTreeEntries(
        IEnumerable<ArchiveDefinition> availableArchiveDefinitions)
    {
        var rootArchiveTreeEntries = new List<ArchiveTreeEntry>();
        rootArchiveTreeEntries.AddRange(
            availableArchiveDefinitions.Where(e => e.TypeOfArchive == ArchiveType.Archive)
                .Select(x => new ArchiveTreeEntry(x)));
        return rootArchiveTreeEntries;
    }

    private static IEnumerable<ArchiveDefinition> GetChildren(
        IEnumerable<ArchiveDefinition> availableArchiveDefinitions,
        string parentId)
    {
        return availableArchiveDefinitions.Where(e => e.ParentId == parentId);
    }

    private static void HandleAvailableEntriesForGivenParent(
        List<ArchiveDefinition> availableArchiveDefinitions,
        ArchiveTreeEntry parent)
    {
        if (availableArchiveDefinitions.Count > 0)
        {
            var children = GetChildren(availableArchiveDefinitions, parent.Id);
            AssignChildrenToParent(parent, children);
            RemoveAssignedItemsFromAvailabeEntries(availableArchiveDefinitions, parent.Id);
            foreach (var nextParent in parent.Children)
            {
                HandleAvailableEntriesForGivenParent(availableArchiveDefinitions, nextParent);
            }
        }
    }

    private static void RemoveAssignedItemsFromAvailabeEntries(
        List<ArchiveDefinition> availableArchiveDefinitions,
        string parentId)
    {
        availableArchiveDefinitions.RemoveAll(e => e.ParentId == parentId);
    }

    private static List<ArchiveDefinition> RemoveRootArchiveDefinitions(
        List<ArchiveDefinition> availableArchiveDefinitions)
    {
        var newEntries =
            availableArchiveDefinitions.Except(
                availableArchiveDefinitions.Where(e => e.TypeOfArchive == ArchiveType.Archive))
                .ToList();
        return newEntries;
    }
}

It could be also an improvement if the enum ArchiveType could be removed, for instance using a base abstract class and three derived classes. — Olorin71, Aug 1 at 18:35
Thanks for your time to review my code. I will check this on monday when I am back in the office. — Heslacher, Aug 2 at 10:19

Jamal · Answer 2 · 2014-08-01 13:52:47Z

You could use the AddRange combined with the Except method inside of Build:

public static List<ArchiveTreeEntry> Build(List<ArchiveDefinition> entries)
{
    List<ArchiveTreeEntry> rootArchiveTreeEntries = new List<ArchiveTreeEntry>();

    if (entries != null && entries.Count > 0)
    {
        List<ArchiveDefinition> rootEntries = GetRootEntries(entries);

        entries = rootArchiveTreeEntried.AddRange(rootEntries.Select(definition=> new ArchiveTreeEntry(definition)).Except(entries);

        foreach (ArchiveTreeEntry parent in rootArchiveTreeEntries)
        {
            FillChildren(parent, entries);
        }
    }
    return rootArchiveTreeEntries;
}

You have made the public method static, which in this case is probably fine but it does make it harder to unit test dependencies. I envisage the class being consumed, like this:

in app
```
var builder = new ArchiveBuilder();  
builder.Build(entities);
```
That said, the private methods are fine as static methods because the are an implementation detail.
The private methods can return/accept IEnumerable<> instead of List<>

From there, you could then re-write the method above to be:

public static List<ArchiveTreeEntry> Build(List<ArchiveDefinition> entries)
{
    List<ArchiveTreeEntry> rootArchiveTreeEntries = new List<ArchiveTreeEntry>();

    if (entries != null && entries.Count > 0)
    {
        entries = rootArchiveTreeEntried.AddRange(GetRootEntries(entries).Select(entry => new ArchiveTreeEntry(definition)).Except(entries);

        foreach (ArchiveTreeEntry parent in rootArchiveTreeEntries)
        {
            FillChildren(parent, entries);
        }
    }
    return rootArchiveTreeEntries;
}

So here is the completely re-written class:

public class ArchiveBuilder
{
    public IEnumerable<ArchiveTreeEntry> Build(IEnumerable<ArchiveDefinition> entries)
    {
        IEnumerable<ArchiveTreeEntry> rootArchiveTreeEntries = new List<ArchiveTreeEntry>();

        if (entries != null && entries.Count > 0)
        {
            entries = rootArchiveTreeEntried.AddRange(GetRootEntries(entries).Select(entry => new ArchiveTreeEntry(definition)).Except(entries);

            foreach (ArchiveTreeEntry parent in rootArchiveTreeEntries)
            {
                FillChildren(parent, entries);
            }
        }
        return rootArchiveTreeEntries;
    }

    private static void FillChildren(ArchiveTreeEntry parent,
                                 IEnumerable<ArchiveDefinition> entries)
    {
        if (entries.Count > 0)
        {
            IEnumerable<ArchiveDefinition> children = GetChildren(entries, parent.Id);

            if (children.Count > 0)
            {
                RemoveChildren(entries, parent.Id);

                foreach (ArchiveDefinition child in children)
                {
                    ArchiveTreeEntry treeEntryChild = new ArchiveTreeEntry(child);
                    parent.AddChild(treeEntryChild);
                    FillChildren(treeEntryChild, entries);
                }
            }
        }
    }

    private static IEnumerable<ArchiveDefinition> GetRootEntries(IEnumerable<ArchiveDefinition> entries)
    {
        return entries.FindAll(e => e.TypeOfArchive == ArchiveType.Archive);
    }

    private static IEnumerable<ArchiveDefinition> GetChildren(IEnumerable<ArchiveDefinition> entries, string parentID)
    {
        return entries.FindAll(e => e.ParentId == parentID);
    }

    private static void RemoveChildren(IEnumerable<ArchiveDefinition> entries, string parentID)
    {
        entries.RemoveAll(e => e.ParentId == parentID);
    }
}

NB: I haven't compiled the class or tested it, so it might not work out the box.

+1 Thanks for taking your time to do a review and also for the Top tip for future. Unfortunately IEnumerable can't be used with FindAll(), RemoveAll() and AddRange() (Count property neither, but there i wouldn't care). AddRange() would be a good idea, but as it seems it won't work together with Except(), as the two lists does contain different types. I will think about using them independently. — Heslacher, Aug 1 at 13:35
Ah yes, my bad. It was more to give you ideas as there is nothing glaring obvious that needs correcting. I'll give my self a pro tip, write it in an IDE not notepad ;) — Stuart Blackler, Aug 1 at 14:59

asked	3 months ago
viewed	260 times
active	3 months ago

current community

your communities

more stack exchange communities

Building tree structure based on flat objects

2 Answers 2

Your Answer

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

Linked

Hot Network Questions

current community

your communities

more stack exchange communities

Building tree structure based on flat objects

2 Answers 2

Your Answer

Sign up or log in

Post as a guest

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

Linked

Related

Hot Network Questions