8
\$\begingroup\$

I'm working on a C# implementation of a tree class. The tree will be used wherever I need to store data in a tree format. (Application Menu, File Infos etc.). Therefore, the tree should support a lot of different use cases.

I think my implementation is ok… but I'm not sure. I never implemented a tree before and I also didn’t find any good examples.

Can you take a look at it and give me some feedback? The source is here: GDrive

I recommend to use the provided visual studio solution (Uploaded to GDrive as zip file).

/// <summary>
///     Class representing a node of a tree.
/// </summary>
/// <typeparam name="T">The type of the value of the node.</typeparam>
public class TreeNode<T> : ITreeNode<T>
{
    #region Fields

    /// <summary>
    ///     The ancestors traversal direction.
    /// </summary>
    private TreeTraversalDirection _ancestorsTraversalDirection;

    /// <summary>
    ///     The children of the node.
    /// </summary>
    private ITreeNodeCollection<T> _children;

    /// <summary>
    ///     The descendants traversal direction.
    /// </summary>
    private TreeTraversalDirection _descendantsTraversalDirection;

    /// <summary>
    ///     The disposable traversal direction.
    /// </summary>
    private TreeTraversalDirection _disposeTraversalDirection;

    /// <summary>
    ///     The parent of the node.
    /// </summary>
    private ITreeNode<T> _parent;

    /// <summary>
    ///     The search traversal direction.
    /// </summary>
    private TreeTraversalDirection _searchTraversalDirection;

    /// <summary>
    ///     The traversal direction.
    /// </summary>
    private TreeTraversalDirection _traversalDirection;

    /// <summary>
    ///     The value of the node.
    /// </summary>
    private T _value;

    #endregion

    #region Ctor

    /// <summary>
    ///     Creates a new instance of the <see cref="TreeNode{T}" /> class.
    /// </summary>
    public TreeNode()
    {
        Initialize(default(T));
    }

    /// <summary>
    ///     Creates a new instance of the <see cref="TreeNode{T}" /> class.
    /// </summary>
    /// <param name="value">The value of the node.</param>
    public TreeNode(T value)
    {
        Initialize(value);
    }

    /// <summary>
    ///     Creates a new instance of the <see cref="TreeNode{T}" /> class.
    /// </summary>
    /// <param name="parent">The parent of the node.</param>
    public TreeNode(ITreeNode<T> parent)
    {
        Initialize(default(T), parent);
    }

    /// <summary>
    ///     Creates a new instance of the <see cref="TreeNode{T}" /> class.
    /// </summary>
    /// <param name="children">The children of the node.</param>
    public TreeNode(ITreeNodeCollection<T> children)
    {
        Initialize(default(T), children: children);
    }

    /// <summary>
    ///     Creates a new instance of the <see cref="TreeNode{T}" /> class.
    /// </summary>
    /// <param name="value">The value of the node.</param>
    /// <param name="children">The children of the node.</param>
    public TreeNode(T value, ITreeNodeCollection<T> children)
    {
        Initialize(value, children: children);
    }

    /// <summary>
    ///     Creates a new instance of the <see cref="TreeNode{T}" /> class.
    /// </summary>
    /// <param name="value">The value of the node.</param>
    /// <param name="parent">The parent of the node.</param>
    public TreeNode(T value, ITreeNode<T> parent)
    {
        Initialize(value, parent);
    }

    /// <summary>
    ///     Creates a new instance of the <see cref="TreeNode{T}" /> class.
    /// </summary>
    /// <param name="value">The value of the node.</param>
    /// <param name="parent">The parent of the node.</param>
    /// <param name="children">The children of the node.</param>
    public TreeNode(T value, ITreeNode<T> parent, ITreeNodeCollection<T> children)
    {
        Initialize(value, parent, children);
    }

    #endregion

    #region Implementation of ITreeNode

    #region Properties

    /// <summary>
    ///     Gets an enumeration of all tree nodes which are above the current node in the tree.
    /// </summary>
    /// <value>An enumeration of all tree nodes which are above the current node in the tree.</value>
    public IEnumerable<ITreeNode<T>> Ancestors
    {
        get { return GetAncestors(); }
    }

    /// <summary>
    ///     Gets or sets the value of the node.
    /// </summary>
    /// <value>The value of the node.</value>
    public T Value
    {
        get { return _value; }
        set
        {
            var oldValue = _value;
            _value = value;

            //Notify the value about it's node, if the value implements ITreeNodeAware
            var treeNodeAware = _value as ITreeNodeAware<T>;
            if (treeNodeAware != null)
                treeNodeAware.Node = this;

            //Notify the old value about the change of it's node (new node is null)
            treeNodeAware = oldValue as ITreeNodeAware<T>;
            if (treeNodeAware != null)
                treeNodeAware.Node = null;
        }
    }

    /// <summary>
    ///     Gets or sets the parent of the node.
    /// </summary>
    /// <remarks>
    ///     Detaches the node from it's old parent and attaches it to it's new parent.
    /// </remarks>
    /// <value>The parent of the node.</value>
    public ITreeNode<T> Parent
    {
        get { return _parent; }
        set { SetParent(value); }
    }

    /// <summary>
    ///     Gets the root of the tree.
    /// </summary>
    /// <value>The root of the tree.</value>
    public ITreeNode<T> Root
    {
        get { return (Parent == null) ? this : Parent.Root; }
    }

    /// <summary>
    ///     Gets or sets the children of the node.
    /// </summary>
    /// <value>The children of the node.</value>
    public ITreeNodeCollection<T> Children
    {
        get { return _children; }
        set
        {
            if (value == _children)
                return;

            if (_children != null)
                _children.ForEach(x => x.SetParent(null, false, false));

            _children = value;
            _children.ForEach(x => x.SetParent(this, false));
        }
    }

    /// <summary>
    ///     Gets or sets the search traversal direction.
    /// </summary>
    /// <value>The search traversal direction.</value>
    public TreeTraversalDirection SearchTraversalDirection
    {
        get { return _searchTraversalDirection; }
        set
        {
            _searchTraversalDirection = value;
            Children.ForEach(x => x.SearchTraversalDirection = value);
        }
    }

    /// <summary>
    ///     Gets or sets the dispose traversal direction.
    /// </summary>
    /// <value>The dispose traversal direction.</value>
    public TreeTraversalDirection DisposeTraversalDirection
    {
        get { return _disposeTraversalDirection; }
        set
        {
            _disposeTraversalDirection = value;
            Children.ForEach(x => x.DisposeTraversalDirection = value);
        }
    }

    /// <summary>
    ///     Gets or sets the ancestors traversal direction.
    /// </summary>
    /// <value>The ancestors traversal direction.</value>
    public TreeTraversalDirection AncestorsTraversalDirection
    {
        get { return _ancestorsTraversalDirection; }
        set
        {
            _ancestorsTraversalDirection = value;
            Children.ForEach(x => x.AncestorsTraversalDirection = value);
        }
    }

    /// <summary>
    ///     Gets or sets the descendants traversal direction.
    /// </summary>
    /// <value>The descendants traversal direction.</value>
    public TreeTraversalDirection DescendantsTraversalDirection
    {
        get { return _descendantsTraversalDirection; }
        set
        {
            _descendantsTraversalDirection = value;
            Children.ForEach(x => x.DescendantsTraversalDirection = value);
        }
    }

    /// <summary>
    ///     Gets or sets the traversal direction used to enumerate the nodes.
    /// </summary>
    /// <value>The traversal direction used to enumerate the nodes.</value>
    public TreeTraversalDirection TraversalDirection
    {
        get { return _traversalDirection; }
        set
        {
            _traversalDirection = value;
            Children.ForEach(x => x.TraversalDirection = value);
        }
    }

    /// <summary>
    ///     Gets the depth of the node.
    /// </summary>
    /// <value>The depth of the node.</value>
    public Int32 Depth
    {
        get { return (Parent == null ? -1 : Parent.Depth) + 1; }
    }

    /// <summary>
    ///     Gets a value indicating whether the node has any children or not.
    /// </summary>
    /// <value>A value indicating whether the node has any children or not.</value>
    public Boolean HasChildren
    {
        get { return Children != null && Children.Any(); }
    }

    /// <summary>
    ///     Gets a value indicating whether the node has a parent or not.
    /// </summary>
    /// <value>A value indicating whether the node has a parent or not.</value>
    public Boolean HasParent
    {
        get { return Parent != null; }
    }

    /// <summary>
    ///     Gets an enumeration of all tree nodes which are below the current node in the tree.
    /// </summary>
    /// <value>An enumeration of all tree nodes which are below the current node in the tree.</value>
    public IEnumerable<ITreeNode<T>> Descendants
    {
        get { return GetDescendants(); }
    }

    #endregion

    #region Methods

    /// <summary>
    ///     Gets the values which matches the given predicate.
    /// </summary>
    /// <remarks>
    ///     Starts the search at the current tree node and traverses down the tree (Direction based on
    ///     <see cref="SearchTraversalDirection" />).
    /// </remarks>
    /// <param name="predicate">The predicate.</param>
    /// <returns>Returns the values which matches the given predicate.</returns>
    public virtual IEnumerable<T> FindValue(Func<ITreeNode<T>, Boolean> predicate)
    {
        var result = new List<T>();

        //Search from top to bottom
        switch (SearchTraversalDirection)
        {
            case TreeTraversalDirection.TopDown:
                //From top to bottom
                if (predicate(this))
                    result.Add(Value);
                Children.ForEach(x => result.AddRange(x.FindValue(predicate)));
                break;
            case TreeTraversalDirection.BottomUp:
                //From bottom to top
                Children.ForEachReverse(x => result.AddRange(x.FindValue(predicate)));
                if (predicate(this))
                    result.Add(Value);
                break;
        }

        return result;
    }

    /// <summary>
    ///     Gets the nodes which matches the given predicate.
    /// </summary>
    /// <remarks>
    ///     Starts the search at the current tree node and traverses down the tree (Direction based on
    ///     <see cref="SearchTraversalDirection" />).
    /// </remarks>
    /// <param name="predicate">The predicate.</param>
    /// <returns>Returns the nodes which matches the given predicate.</returns>
    public virtual IEnumerable<ITreeNode<T>> FindNode(Func<ITreeNode<T>, Boolean> predicate)
    {
        var result = new List<ITreeNode<T>>();

        //Search from top to bottom
        switch (SearchTraversalDirection)
        {
            case TreeTraversalDirection.TopDown:
                //From top to bottom
                if (predicate(this))
                    result.Add(this);
                Children.ForEach(x => result.AddRange(x.FindNode(predicate)));
                break;
            case TreeTraversalDirection.BottomUp:
                //From bottom to top
                Children.ForEachReverse(x => result.AddRange(x.FindNode(predicate)));
                if (predicate(this))
                    result.Add(this);
                break;
        }

        return result;
    }

    /// <summary>
    ///     Gets the nodes with the given value.
    /// </summary>
    /// <param name="value">The value to search.</param>
    /// <returns>Returns the nodes with the given value.</returns>
    public IEnumerable<ITreeNode<T>> FindNode(T value)
    {
        var result = new List<ITreeNode<T>>();

        //Search from top to bottom
        switch (SearchTraversalDirection)
        {
            case TreeTraversalDirection.TopDown:
                //From top to bottom
                if (Value.Equals(value))
                    result.Add(this);
                Children.ForEach(x => result.AddRange(x.FindNode(value)));
                break;
            case TreeTraversalDirection.BottomUp:
                //From bottom to top
                Children.ForEachReverse(x => result.AddRange(x.FindNode(value)));
                if (Value.Equals(value))
                    result.Add(this);
                break;
        }

        return result;
    }

    /// <summary>
    ///     Adds the given value as child to the node.
    /// </summary>
    /// <param name="value">The value to add.</param>
    /// <returns>Returns the added node.</returns>
    public ITreeNode<T> Add(T value)
    {
        var node = new TreeNode<T>(value, this)
        {
            DisposeTraversalDirection = DisposeTraversalDirection,
            SearchTraversalDirection = SearchTraversalDirection
        };
        Children.Add(node);
        return node;
    }

    /// <summary>
    ///     Adds the given node as child to the node, if it is not already a child of the node.
    /// </summary>
    /// <param name="node">The node to add.</param>
    /// <returns>Returns the added node.</returns>
    public ITreeNode<T> Add(ITreeNode<T> node)
    {
        if (!Children.Contains(node))
            Children.Add(node);

        return node;
    }

    /// <summary>
    ///     Sets the parent of the tree node.
    /// </summary>
    /// <param name="parent">The new parent.</param>
    /// <param name="attacheToNewParent">
    ///     A value determining whether the node should add it self to the children of the new parent
    ///     or not.
    /// </param>
    /// <param name="detachFromOldParent">A value indicating whether the node should detach itself from it's old parent or not.</param>
    public void SetParent(ITreeNode<T> parent,
                           Boolean attacheToNewParent = true,
                           Boolean detachFromOldParent = true)
    {
        if (_parent == parent)
            return;

        //Switch parent
        var oldParent = _parent;
        _parent = parent;

        //Remove node from old parent
        if (oldParent != null && detachFromOldParent)
            oldParent.Children.Remove(this, false);

        if (attacheToNewParent && Parent != null)
            Parent.Children.Add(this, false);
    }

    /// <summary>
    ///     Sets all directions (<see cref="DisposeTraversalDirection" />, <see cref="SearchTraversalDirection" />,
    ///     <see cref="AncestorsTraversalDirection" />, <see cref="DescendantsTraversalDirection" />).
    /// </summary>
    /// <param name="direction">The new direction.</param>
    public void SetAllDirections(TreeTraversalDirection direction)
    {
        SearchTraversalDirection = direction;
        DisposeTraversalDirection = direction;
        AncestorsTraversalDirection = direction;
        DescendantsTraversalDirection = direction;
        TraversalDirection = direction;
    }

    #endregion

    #endregion

    #region Implementation of IEnumerable

    /// <summary>
    ///     Returns an enumerator that iterates through the collection.
    /// </summary>
    /// <returns>
    ///     A <see cref="System.Collections.Generic.IEnumerator{T}" /> that can be used to iterate through the collection.
    /// </returns>
    public IEnumerator<ITreeNode<T>> GetEnumerator()
    {
        return GetEnumeratorInternal()
            .GetEnumerator();
    }

    /// <summary>
    ///     Returns an enumerator that iterates through the collection.
    /// </summary>
    /// <returns>
    ///     A <see cref="System.Collections.IEnumerator" /> that can be used to iterate through the collection.
    /// </returns>
    IEnumerator IEnumerable.GetEnumerator()
    {
        return GetEnumerator();
    }

    #endregion

    #region Overrides of Object

    /// <summary>
    ///     Returns a string that represents the current object.
    /// </summary>
    /// <returns>
    ///     A string that represents the current object.
    /// </returns>
    public override String ToString()
    {
        return "Depth: {0} - Value: {1}, Children: {2}, Parent: {{{3}}}"
            .F(Depth,
                Value == null
                    ? "[NULL]"
                    : Value.ToString(),
                Children.Count,
                Parent == null
                    ? "[NULL]"
                    : Parent.ToString());
    }

    #endregion

    #region Private Members

    /// <summary>
    ///     Internal implementation to get a enumeration in the specified order.
    /// </summary>
    /// <returns>Returns a  enumeration of all nodes.</returns>
    private IEnumerable<ITreeNode<T>> GetEnumeratorInternal()
    {
        switch (TraversalDirection)
        {
            case TreeTraversalDirection.TopDown:
                yield return this;
                foreach (var child in Children)
                {
                    if (child is TreeNode<T> == false)
                        throw new NotSupportedException("Child '{0}' is not of type TreeNode{T}.".F(child));

                    var enumeration = (child as TreeNode<T>).GetEnumeratorInternal();
                    foreach (var e in enumeration)
                        yield return e;
                }

                break;
            case TreeTraversalDirection.BottomUp:

                foreach (var child in Children.Reverse())
                {
                    if (child is TreeNode<T> == false)
                        throw new NotSupportedException("Child '{0}' is not of type TreeNode{T}.".F(child));

                    var enumeration = (child as TreeNode<T>).GetEnumeratorInternal();
                    foreach (var e in enumeration)
                        yield return e;
                }
                yield return this;

                break;
            default:
                throw new ArgumentOutOfRangeException(this.GetName(() => TraversalDirection),
                                                       "The value '{0}' is a unknown tree traversal direction".F(
                                                           TraversalDirection));
        }
    }

    private IEnumerable<ITreeNode<T>> GetDescendants(List<ITreeNode<T>> descendants = null)
    {
        descendants = descendants ?? new List<ITreeNode<T>>();
        if (Children == null || !Children.Any())
            return descendants;

        Children.ForEach(x =>
        {
            if (x is TreeNode<T> == false)
                throw new NotSupportedException("Child '{0}' is not of type TreeNode{{T}}.".F(x));

            descendants.Add(x);
            (x as TreeNode<T>).GetDescendants(descendants);
        });

        return descendants;
    }

    private IEnumerable<ITreeNode<T>> GetAncestors(List<ITreeNode<T>> ancestors = null)
    {
        ancestors = ancestors ?? new List<ITreeNode<T>>();
        if (Parent == null)
            return ancestors;

        if (Parent is TreeNode<T> == false)
            throw new NotSupportedException("Parent is not of type TreeNode{T}.");

        ancestors.Add(Parent);
        return (Parent as TreeNode<T>).GetAncestors(ancestors);
    }

    private void Initialize(T value,
                             ITreeNode<T> parent = null,
                             ITreeNodeCollection<T> children = null)
    {
        Value = value;
        Parent = parent;
        Children = children ?? new TreeNodeCollection<T>(this);
        if (Parent != null)
        {
            if (!Parent.Children.Contains(this))
                Parent.Children.Add(this);

            DisposeTraversalDirection = Parent.DisposeTraversalDirection;
            SearchTraversalDirection = Parent.SearchTraversalDirection;
            AncestorsTraversalDirection = Parent.AncestorsTraversalDirection;
            DescendantsTraversalDirection = Parent.DescendantsTraversalDirection;
        }
        else
        {
            DisposeTraversalDirection = TreeTraversalDirection.BottomUp;
            SearchTraversalDirection = TreeTraversalDirection.BottomUp;
            AncestorsTraversalDirection = TreeTraversalDirection.BottomUp;
            DescendantsTraversalDirection = TreeTraversalDirection.BottomUp;
        }
    }

    #endregion

    #region IDisposable

    /// <summary>
    ///     Release all resources hold by the node.
    /// </summary>
    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    /// <summary>
    ///     Destructs the tree node..
    /// </summary>
    ~TreeNode()
    {
        Dispose(false);
    }

    /// <summary>
    ///     Releases the managed and unmanaged resource hold by the node.
    /// </summary>
    /// <param name="disposing">A value of true to release managed resources, false to release unmanaged resources.</param>
    protected virtual void Dispose(Boolean disposing)
    {
        if (!disposing)
            return;

        //Release from bottom up (start with children).
        if (DisposeTraversalDirection == TreeTraversalDirection.BottomUp)
            foreach (var node in Children.Reverse())
                node.Dispose();

        //Release the current node.
        var dispose = Value as IDisposable;
        if (dispose != null)
            (Value as IDisposable).Dispose();

        //Check if children are released or not.
        if (DisposeTraversalDirection != TreeTraversalDirection.TopDown)
            return;

        //Release from top down (start with current node).
        foreach (var node in Children)
            node.Dispose();
    }

    #endregion
}

public class TreeNodeCollection<T> : Collection<ITreeNode<T>>, ITreeNodeCollection<T>
{
    #region Fields

    private ITreeNode<T> _parent;

    #endregion

    #region Ctor

    public TreeNodeCollection(ITreeNode<T> parent)
    {
        _parent = parent;
    }

    #endregion

    #region Implementation of ITreeNodeCollection<T>

    #region Properties

    public ITreeNode<T> Parent
    {
        get { return _parent; }
        set
        {
            if (_parent == value)
                return;

            if (_parent != null)
                _parent.Children = new TreeNodeCollection<T>(_parent);
            _parent = value;
            if (_parent != null)
            {
                _parent.Children.DetachFromParent();
                _parent.Children = this;
            }
            this.ForEach(x => x.SetParent(_parent, false));
        }
    }

    #endregion

    #region Methods

    /// <summary>
    ///     Adds the given value as new node to the list and sets it's parent to the parent of the list.
    /// </summary>
    /// <param name="value">The value to add.</param>
    /// <returns>Returns the new created node.</returns>
    public ITreeNode<T> Add(T value)
    {
        var node = new TreeNode<T>(value);
        Add(node);

        return node;
    }

    /// <summary>
    ///     Detaches the collection and all it's items form it's current parent.
    /// </summary>
    public void DetachFromParent()
    {
        _parent = null;
        this.ForEach(x => x.Parent = null);
    }

    /// <summary>
    ///     Adds the given item to the list and sets it's parent to the parent of the list.
    /// </summary>
    /// <exception cref="ArgumentNullException">item can not be null.</exception>
    /// <param name="item">The item to add.</param>
    /// <param name="setParent">
    ///     A value indicating weather the parent of the given item should be set to the parent of the
    ///     collection or not.
    /// </param>
    public void Add(ITreeNode<T> item, Boolean setParent)
    {
        item.ThrowIfNull(() => item);

        if (Contains(item))
            return;

        base.Add(item);
        if (setParent)
            item.Parent = Parent;
    }

    public Boolean Remove(ITreeNode<T> item, Boolean setParent)
    {
        item.ThrowIfNull(() => item);

        var result = base.Remove(item);
        if (result && setParent)
            item.Parent = null;

        return result;
    }

    #endregion

    #endregion

    #region Implementation of ICollection<ITreeNode<T>>

    public new void Add(ITreeNode<T> item)
    {
        Add(item, true);
    }

    public new Boolean Remove(ITreeNode<T> item)
    {
        return Remove(item, true);
    }

    #endregion

    #region Overrides of Object

    public override String ToString()
    {
        return "Count: {0}, Parent: {{{1}}}".F(Count, Parent == null ? "[NULL]" : Parent.ToString());
    }

    #endregion
}
\$\endgroup\$
1
  • 3
    \$\begingroup\$ You don't have to add the whole project to the solution, just the code you want reviewed. What you have now looks good. \$\endgroup\$
    – Snowbody
    Commented Apr 2, 2015 at 23:50

3 Answers 3

7
\$\begingroup\$

Comments

Comments should clarify the code, not simply restate it. You have a lot of comments like this.

   /// <summary>
   ///     The parent of the node.
   /// </summary>
   private ITreeNode<T> _parent;

It's redundant and noisy. They actually make the code hard to read in my opinion. Secondly, I love doc comments. They're great, but I see no value in placing them on private members. If a private member needs a comment, use a regular one. Or even better, refactor/rename so that a comment is no longer needed.

Constructors

You have a lot of them and they tend to duplicate functionality. If you changed one of them, you'd have to change many of them. Use CTor chaining instead. Every other constructor should call on this one. The one that takes in every possible argument.

   public TreeNode(T value, ITreeNode<T> parent, ITreeNodeCollection<T> children)
   {
       Initialize(value, parent, children);
   }

Then you chain it like this.

    /// <summary>
    ///     Creates a new instance of the <see cref="TreeNode{T}" /> class.
    /// </summary>
    /// <param name="children">The children of the node.</param>
    public TreeNode(ITreeNodeCollection<T> children)
        :this(default(T), children: children)
    { }

Note that utilizing this strategy, you could likely implement all of the Initialize() logic in one ctor, where it belongs.

Braces

Use them. Always. Take a look at this snippet.

       set
       {
           if (value == _children)
               return;

           if (_children != null)
               _children.ForEach(x => x.SetParent(null, false, false));

           _children = value;
           _children.ForEach(x => x.SetParent(this, false));
       }

Does _children = value execute if children != null, or always? It executes always, but we have to think about it. Don't make me think. Use braces instead. The second thing to consider is that it is likely you will add to/modify the set parent logic that happens if _children isn't null. The second you add another line of code to that logic, you'll need to add the braces anyway. If you forget to, then you've got a bug that didn't need to happen.

Actually, upon further inspection, this method may not be doing what you intended it to do. Minimally, it's doing something silly. If children isn't null, the collection gets looped through twice; setting and then resetting their parents. Double check this property.

Misc

This was an interesting design decision. I'm not sure whether I like it or not.

public ITreeNode<T> Root
{
    get { return (Parent == null) ? this : Parent.Root; }
}

I'm not sure I would expect the root node to return itself. I guess it makes sense and it does remove any need to null check the root. So, yeah. I guess I do like this after all. Well done.


It's not an entirely thorough review, but that should give you a good start until someone else comes along.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Thank you very much for the review. You’ve right the Children property contains a ‘bug’. I’ve changed this. I created the Initialize method because I like the constructor to be as sort as possible, and I didn’t chained the constructors to reduce the number of method calls. But I will change this (Move logic from Initialize to Ctor). I implemented the Root property like this, because I distinguish Root and Parent. \$\endgroup\$ Commented Apr 3, 2015 at 13:50
  • \$\begingroup\$ You're very welcome. I'm happy that I could help. \$\endgroup\$
    – RubberDuck
    Commented Apr 3, 2015 at 13:52
5
\$\begingroup\$
private IEnumerable<ITreeNode<T>> GetAncestors(List<ITreeNode<T>> ancestors = null)
{
    ancestors = ancestors ?? new List<ITreeNode<T>>();
    if (Parent == null)
        return ancestors;

    if (Parent is TreeNode<T> == false)
        throw new NotSupportedException("Parent is not of type TreeNode{T}.");

    ancestors.Add(Parent);
    return (Parent as TreeNode<T>).GetAncestors(ancestors);
}

I don't see the need to check that each ancestor is of type TreeNode<T> here. If we remove that check, I would write

private IEnumerable<ITreeNode<T>> GetAncestors()
{
    for (var ancestor = this.Parent; ancestor != null; ancestor = ancestor.Parent)
    {
        yield return ancestor;
    }
}

If we introduce an optional parameter, includeSelf, we can write Root in terms of GetAncestor:

private IEnumerable<ITreeNode<T>> GetAncestors(bool includeSelf = false)
{
    var ancestor = includeSelf ? this : this.Parent;
    while (ancestor != null)
    {
        yield return ancestor;
        ancestor = ancestor.Parent;
    }
}

public ITreeNode<T> Root
{
    get { return GetAncestors(includeSelf: true).Last(); }
}
\$\endgroup\$
1
  • \$\begingroup\$ Thank you very much for the review. You’re absolutely right. ‘GetAncestors’ could be implemented with way less code than it is now. I will change this. \$\endgroup\$ Commented Apr 7, 2015 at 16:58
1
\$\begingroup\$

Literature

Since you never worked with a Tree structure before, I recommend reading about tree traversal strategies.


Bug Alert

If you set the same value as is already set and the value is ITreeNodeAware<T>, it will end up with Node is null, while it should have the current instance set. This could be solved by either only setting a value if it differs from the current value, or to swap the order of the treeNodeAware.Node assignments.

public T Value
{
    get { return _value; }
    set
    {
        var oldValue = _value;
        _value = value;

        //Notify the value about it's node, if the value implements ITreeNodeAware
        var treeNodeAware = _value as ITreeNodeAware<T>;
        if (treeNodeAware != null)
            treeNodeAware.Node = this;

        //Notify the old value about the change of it's node (new node is null)
        treeNodeAware = oldValue as ITreeNodeAware<T>;
        if (treeNodeAware != null)
            treeNodeAware.Node = null;
    }
}

Fix:

public T Value
{
    get => _value;
    set
    {
        if (value != _value) 
        {
            UpdateNode(_value, null);
            _value = value;
            UpdateNode(_value, this);
        }
    }
}

private void UpdateNode(T element, TreeNode<T> node)
{
    if (element is ITreeNodeAware<T> treeNodeAware)
    {
        treeNodeAware.Node = node;
    }
}

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.