Skip to main content
1 of 4
dfhwze
  • 14.2k
  • 3
  • 40
  • 101

Review

There is a bug with IsRoot. Also, why not provide a property Root { get; }?

if the parent is the root of the tree, then the parent is set to null

public bool IsRoot { get { return Parent != null; } }

You should also take advantage of the sweet syntactic sugar of the language (for all your properties):

 public bool IsRoot => Parent == null;

Since Children is private and you always instantiate a list, there is no reason to use null-propagation here:

public int Count { get { return Children?.Count ?? 0; } }

public int Count => Children.Count;

AddChild should throw exceptions on invalid input. You don't check for an invalid tree, what if the node is a grand-parent of the the current instance? Perform similar checks for RemoveChild.

public void AddChild(Node node)
{
    node = node ?? throw new ArgumentNullException(nameof(node));
    if (IsAncestorOrSelf(node)) // <- you should implement such method
        throw new ArgumentException("The node can not be an ancestor or self");
    node.Parent = this;
    Children.Add(node);
}

GetChildren should return an immutable copy of the list containing the children.

public IEnumerable<Node> GetChildren()
{
    return Children.ToArray();
}

I don't know why you would need DeepCopy functionality.


GetChildrenRecursive should be called GetDescendants. I would implement it using recursion.

public IEnumerable<Node> GetDescendants()
{
    foreach (var child in Children)
    {
         yield return child;
         foreach (var descendant in child.GetDescendants())
         {
              yield return descendant;
         }
    }
}
dfhwze
  • 14.2k
  • 3
  • 40
  • 101