patterncsharpMinor
C# Tree implementation
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
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.
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
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.
Then you chain it like this.
Note that utilizing this strategy, you could likely implement all of the
Braces
Use them. Always. Take a look at this snippet.
Does
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.
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.
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.