HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

C# Tree implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
implementationtreestackoverflow

Problem

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).

```
///
/// Class representing a node of a tree.
///
/// The type of the value of the node.
public class TreeNode : ITreeNode
{
#region Fields

///
/// The ancestors traversal direction.
///
private TreeTraversalDirection _ancestorsTraversalDirection;

///
/// The children of the node.
///
private ITreeNodeCollection _children;

///
/// The descendants traversal direction.
///
private TreeTraversalDirection _descendantsTraversalDirection;

///
/// The disposable traversal direction.
///
private TreeTraversalDirection _disposeTraversalDirection;

///
/// The parent of the node.
///
private ITreeNode _parent;

///
/// The search traversal direction.
///
private TreeTraversalDirection _searchTraversalDirection;

///
/// The traversal direction.
///
private TreeTraversalDirection _traversalDirection;

///
/// The value of the node.
///
private T _value;

#endregion

#region Ctor

///
/// Creates a new instance of the class.
///
public TreeNode()
{
Initialize(default(T));
}

///
/// Creates a new instance of the class.
///
/// The value of the node.
public TreeNode(T value)
{
Initialize(value);
}

///
/// Creates a new instance of the class.
///
/// The parent

Solution

Comments

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

/// 
   ///     The parent of the node.
   /// 
   private ITreeNode _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 parent, ITreeNodeCollection children)
   {
       Initialize(value, parent, children);
   }


Then you chain it like this.

/// 
    ///     Creates a new instance of the  class.
    /// 
    /// The children of the node.
    public TreeNode(ITreeNodeCollection 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 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.

Code Snippets

/// <summary>
   ///     The parent of the node.
   /// </summary>
   private ITreeNode<T> _parent;
public TreeNode(T value, ITreeNode<T> parent, ITreeNodeCollection<T> children)
   {
       Initialize(value, parent, children);
   }
/// <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)
    { }
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));
       }
public ITreeNode<T> Root
{
    get { return (Parent == null) ? this : Parent.Root; }
}

Context

StackExchange Code Review Q#85734, answer score: 7

Revisions (0)

No revisions yet.