Building tree structure based on flat objects - follow up

Question

This is a follow up on this question and has now a follow up question here.

I have implemented some changes to the class below and also added the method AddChildRange() to the ArchiveTreeEntry class.
I have kept the method as static, as this method is the only purpose of this class, and none of the passed items will be changed.
I am a little bit unsure about the methodname RemoveChildren.
Please review the class in question if you find anything which should be changed.

The class in question

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 = RemoveRootArchiveDefinitions(localEntries);

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

        return rootArchiveTreeEntries;
    }

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

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

        return rootArchiveTreeEntries;
    }
    private static IEnumerable<ArchiveDefinition> RemoveRootArchiveDefinitions(
        IEnumerable<ArchiveDefinition> archiveDefinitions)
    {
        IEnumerable<ArchiveDefinition> newEntries =
                archiveDefinitions.Except(
                archiveDefinitions.Where(e => e.TypeOfArchive == ArchiveType.Archive));

        return newEntries;
    }

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

            AddChildrenToParent(parent, children);

            RemoveChildren(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 void RemoveChildren(
        IEnumerable<ArchiveDefinition> archiveDefinition,
        string parentId)
    {
        archiveDefinition.Select(e => e.ParentId != parentId);
    }

}

Related changed class

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

    internal void AddChildRange(IEnumerable<ArchiveTreeEntry> children)
    {
        if (children != null)
        {
            mChildren.AddRange(children);
        }
    }
}

EDIT: As the RemoveChildren method doesn't do, what I thought it will do, please skip it in your review.

Answer 1

I don't see much wrong, but then again, I don't have much C# knowledge. One thing I do spot however, is the following:

archiveDefinitions.Count() > 0 might be replacable with archiveDefinitions.Any().

See this code snippet in MSDN documentation for usage:

List<int> numbers = new List<int> { 1, 2 };
bool hasElements = numbers.Any();

Console.WriteLine("The list {0} empty.",
    hasElements ? "is not" : "is");

// This code produces the following output: 
// 
// The list is not empty. 

By using the Any() method, you prevent counting ALL the elements. This should increase performance, which is an improvement of the code. I don't know whether use of Any() for such a thing is common in C#, though. If it isn't, you might want to add a comment.

Additionally, in RemoveRootArchiveDefinitions, you name the return variable newEntries. Shouldn't it be removedEntries? If not, shouldn't the method name be changed?

Answer 2

ArchiveTreeEntry

Returning early will enhance readability. So the AddChild() and AddChildRange() method should look like so

internal void AddChild(ArchiveTreeEntry child)
{

    if (child == null) { return; }

    mChildren.Add(child);
}

an additional check with !Any() will make the programm flow more prominent because it clearly states to add only if ther are any items to add.

internal void AddChildRange(IEnumerable<ArchiveTreeEntry> children)
{

    if (children == null || !children.Any())  { return; }

    mChildren.AddRange(children);

}  

---

ArchiveBuilder

• In the CreateRootEntries() method you have a copy&pasta bug. The return statement says return rootArchiveTreeEntries; instead of return rootEntries;

• you really should use the var type if it is obvious from the right hand side of the assignment which type is assigned like for instance here

  List<ArchiveTreeEntry> rootEntries = new List<ArchiveTreeEntry>();  
  

like so

    var rootEntries = new List<ArchiveTreeEntry>();  

---

Implementing the mentioned points together with the mentioned points from @Pimgd answer will lead to

public class ArchiveBuilder
{
    public static IEnumerable<ArchiveTreeEntry> Build(IEnumerable<ArchiveDefinition> archiveDefinitions)
    {
        if (archiveDefinitions == null || !archiveDefinitions.Any()) { return Enumerable.Empty<ArchiveTreeEntry>(); }

        IEnumerable<ArchiveDefinition> localEntries = new List<ArchiveDefinition>(archiveDefinitions);

        var rootArchiveTreeEntries = new List<ArchiveTreeEntry>(CreateRootEntries(localEntries));

        localEntries = RemoveRootArchiveDefinitions(localEntries);

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

        return rootArchiveTreeEntries;
    }

    private static IEnumerable<ArchiveTreeEntry> CreateRootEntries(
        IEnumerable<ArchiveDefinition> archiveDefinitions)
    {

        var rootEntries = new List<ArchiveTreeEntry>();

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

        return rootEntries;
    }

    private static IEnumerable<ArchiveDefinition> RemoveRootArchiveDefinitions(
        IEnumerable<ArchiveDefinition> archiveDefinitions)
    {

        return archiveDefinitions.Except(
                archiveDefinitions.Where(e => e.TypeOfArchive == ArchiveType.Archive));

    }


    private static void HandleEntriesForParent(
        IEnumerable<ArchiveDefinition> archiveDefinitions,
        ArchiveTreeEntry parent)
    {

        if (!archiveDefinitions.Any()) { return; }

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

        AddChildrenToParent(parent, children);

        RemoveChildren(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 void RemoveChildren(
        IEnumerable<ArchiveDefinition> archiveDefinition,
        string parentId)
    {
        archiveDefinition.Select(e => e.ParentId != parentId);
    }
}