Skip to main content
Code Review

Return to Revisions

2 of 10
added 41 characters in body
user avatar
user avatar 
public Node DeepCopy()
{
    var other = (Node)MemberwiseClone();

    other.Children = new List<Node>(collection: Children);
    other.Parent = Parent?.DeepCopy();
    other.Value = new Node(value: Value);

    return other;
}

You should be careful when using this, because it actually clones the entire tree (via Parent and Children). Besides that, I think MemberwiseClone copies the Children and Parent recursively. So by creating a new list for the Children and calling DeepCopy() on the Parent you actually get a mix of copies and existing Node objects, that can lead to unexpected behavior if you change either the copy or the original later on. And the child instances (other) will not be part of the parents Children list in the copy.

Why does other.Value becomes a Node(Value)? - Value is by the way also covered by MemberwiseClone.

Consider if it is of any use and possibly skip it. I can't see any use of it?

public void RemoveChild(Node node)
{
    if (node != this && Children.Contains(node))
    {
        Children.Remove(node);
    }
}

It is safe to call Children.Remove(node) even if node is null, so you can spare the Contains() check. node != this - I suppose this should be avoided in the Add method - but what can't this be removed if provided as node? You could consider to return the bool values returned from Children.Remove(node), to let the client known if the operation was succesful or not.

You could consider to make the Node class generic:

public class Node<T>
{
    public T Value { get; }
    ...
}

Where is the Search algorithm to review? :-)

user73941