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?

• Pimgd: Ah, okay. Another question, why do you call the return value of RemoveRootArchiveDefinitions newEntries? Shouldn't it be something like removedEntries?

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