Building tree structure based on flat objects - second follow up

Question

This is a follow up on this question which is a follow up on this question
This follow up is created, because the class in question contained something, which didn't do what it should do.

Based on the suggestion for the RemoveRootArchiveDefinitions() method of the first follow up I have renamed the name of the return variable to remainingEntries and renamed the method to GetNonRootArchiveDefinitions().

Please review the class in question, if you find anything, which should be changed.

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

        if (archiveDefinitions != null && archiveDefinitions.Count() > 0)
        {
            IEnumerable<ArchiveDefinition> localEntries = new List<ArchiveDefinition>(archiveDefinitions);

            rootArchiveTreeEntries = CreateRootEntries(localEntries);
            localEntries = GetNonRootArchiveDefinitions(localEntries);

            foreach (ArchiveTreeEntry rootEntry in rootArchiveTreeEntries)
            {
                HandleEntriesForParent(localEntries, rootEntry);
            }
        }

        return rootArchiveTreeEntries;
    }

    private static IEnumerable<ArchiveTreeEntry> CreateRootEntries(
        IEnumerable<ArchiveDefinition> archiveDefinitions)
    {
        List<ArchiveTreeEntry> rootArchiveTreeEntries = new List<ArchiveTreeEntry>();

        rootArchiveTreeEntries.AddRange(
            archiveDefinitions.Where(e => e.TypeOfArchive == ArchiveType.Archive)
                .Select(d => new ArchiveTreeEntry(d)));

        return rootArchiveTreeEntries;
    }

    private static IEnumerable<ArchiveDefinition> GetNonRootArchiveDefinitions(
        IEnumerable<ArchiveDefinition> archiveDefinitions)
    {
        IEnumerable<ArchiveDefinition> remainingEntries =
                archiveDefinitions.Except(
                archiveDefinitions.Where(e => e.TypeOfArchive == ArchiveType.Archive));

        return remainingEntries;
    }

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

            AddChildrenToParent(parent, children);

            archiveDefinitions = GetParentlessEntries(archiveDefinitions, parent.Id);

            foreach (ArchiveTreeEntry nextParent in parent.Children)
            {
                HandleEntriesForParent(archiveDefinitions, nextParent);
            }
        }
    }

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

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

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

}

Regarding Any() and Count() as pointed out in the last follow up:

As long as the underlying object is a ICollection or ICollection<T> the Count() method returns the Count property of the underlying object. Which in turn just returns an internal counter of the items contained in the collection.

See: this question and this question

Answer 1

As long as the underlying object is a ICollection or ICollection<T> the Count() method returns the Count property of the underlying object.

That's correct, but since your parameter has type IEnumerable<T>, and the method is public, you have no guarantee that the underlying collection is an ICollection<T>. Even in non-public methods, if you assume the underlying collection is an ICollection<T>, your method signature should reflect that; if you decide to accept the more generic IEnumerable<T>, then you should use Any(). Otherwise you have hidden assumptions throughout your code that the type system cannot help with.

---

Use of the var keyword would clean things up considerably.

---

In this code

private static IEnumerable<ArchiveTreeEntry> CreateRootEntries(
    IEnumerable<ArchiveDefinition> archiveDefinitions)
{
    List<ArchiveTreeEntry> rootArchiveTreeEntries = new List<ArchiveTreeEntry>();

    rootArchiveTreeEntries.AddRange(
        archiveDefinitions.Where(e => e.TypeOfArchive == ArchiveType.Archive)
            .Select(d => new ArchiveTreeEntry(d)));

    return rootArchiveTreeEntries;
}

there's no point creating a List<T>, you can just write

private static IEnumerable<ArchiveTreeEntry> CreateRootEntries(
    IEnumerable<ArchiveDefinition> archiveDefinitions)
{
    return archiveDefinitions.Where(e => e.TypeOfArchive == ArchiveType.Archive)
        .Select(d => new ArchiveTreeEntry(d));
}

---

Similarly, you can write

private static IEnumerable<ArchiveDefinition> GetNonRootArchiveDefinitions(
    IEnumerable<ArchiveDefinition> archiveDefinitions)
{
    IEnumerable<ArchiveDefinition> remainingEntries =
            archiveDefinitions.Except(
            archiveDefinitions.Where(e => e.TypeOfArchive == ArchiveType.Archive));

    return remainingEntries;
}

as

private static IEnumerable<ArchiveDefinition> GetNonRootArchiveDefinitions(
    IEnumerable<ArchiveDefinition> archiveDefinitions)
{
    return archiveDefinitions.Where(e => e.TypeOfArchive != ArchiveType.Archive);
}

Answer 2

I'm a bit of a nut, and really like extension method syntax. So I employed some of mjolka's refactorings, put in a few minor ones of my own (primarily around variable naming) and provided an extension method veneer on top of it:

public static class ArchiveBuilder
{
    public static IEnumerable<ArchiveTreeEntry> Build(this IEnumerable<ArchiveDefinition> archiveDefinitions)
    {
        archiveDefinitions = archiveDefinitions as IList<ArchiveDefinition> ?? archiveDefinitions.ToList();
        if (!archiveDefinitions.Any())
        {
            return Enumerable.Empty<ArchiveTreeEntry>();
        }

        IEnumerable<ArchiveDefinition> localEntries = new List<ArchiveDefinition>(archiveDefinitions);
        var rootArchiveTreeEntries = localEntries.CreateRootEntries().ToList();

        localEntries = localEntries.GetNonRootArchiveDefinitions();
        foreach (var rootEntry in rootArchiveTreeEntries)
        {
            localEntries.HandleEntriesForParent(rootEntry);
        }

        return rootArchiveTreeEntries;
    }

    private static IEnumerable<ArchiveTreeEntry> CreateRootEntries(
        this IEnumerable<ArchiveDefinition> archiveDefinitions)
    {
        return archiveDefinitions
            .Where(archiveDefinition => archiveDefinition.TypeOfArchive == ArchiveType.Archive)
            .Select(archiveDefinition => new ArchiveTreeEntry(archiveDefinition));
    }

    private static IEnumerable<ArchiveDefinition> GetNonRootArchiveDefinitions(
        this IEnumerable<ArchiveDefinition> archiveDefinitions)
    {
        return archiveDefinitions
            .Where(archiveDefinition => archiveDefinition.TypeOfArchive != ArchiveType.Archive);
    }

    private static void HandleEntriesForParent(
        this IEnumerable<ArchiveDefinition> archiveDefinitions,
        ArchiveTreeEntry parent)
    {
        archiveDefinitions = archiveDefinitions as IList<ArchiveDefinition> ?? archiveDefinitions.ToList();
        if (!archiveDefinitions.Any())
        {
            return;
        }

        var children = archiveDefinitions.GetChildren(parent.Id);

        parent.AddChildrenToParent(children);
        archiveDefinitions = archiveDefinitions.GetParentlessEntries(parent.Id);
        foreach (var nextParent in parent.Children)
        {
            archiveDefinitions.HandleEntriesForParent(nextParent);
        }
    }

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

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

    private static IEnumerable<ArchiveDefinition> GetParentlessEntries(
        this IEnumerable<ArchiveDefinition> archiveDefinitions,
        string parentId)
    {
        return archiveDefinitions.Where(archiveDefinition => archiveDefinition.ParentId != parentId);
    }
}