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

X'mas tree implementation

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

Problem

With the festive season mood and the new year coming, I thought I would make my first question this year, a tree implementation. Blame me because it doesn't have anything to do about Christmas.

```
public interface ITreeITem
{
object Key { get; }
object ParentKey { get; set; }
}

public class Tree : IEnumerable
where T: ITreeITem
{
private readonly IDictionary> _items = new Dictionary>(7);
public T Item { get; set; }
public Tree()
{
}

public Tree(IEnumerable values)
{
foreach (var value in values)
{
if (value.ParentKey == null)
{
AddAsSubTree(value);
}
else
{
Parent(value)?.AddAsSubTree(value);
}
}
}

public IEnumerable> Children
{
get { return _items.Values; }
}

public Tree Parent(T item)
{
if (item.ParentKey == null)
{
return this;
}
if(_items.Count == 0)
{
return null;
}
var child = _items.TryGetOrValue(item.ParentKey, null);
if (child == null)
{
return _items.Values
.Select(v => v.Parent(item))
.FirstOrDefault(v => v != null);
}
else
{
return child;
}
}

public Tree Parent(T item, int level)
{
if (level == 1)
{
return this;
}
return ChildrenInLevel(level - 1)
.FirstOrDefault(t => t._items.ContainsKey(item.ParentKey));
}

protected Tree AddAsSubTree(T item)
{
var tree = new Tree
{
Item = item
};
_items.Add(item.Key, tree);
return tree;
}

protected IEnumerable AllChildren(Tree root)
{
foreach (var item in root._items.Values)
{
yield return item.Item;
foreach (var aux in AllChildren(item))

Solution

-
The Tree constructor silently discards items where the parent can't be found. This is usually a bad idea since it means the caller has no real idea why items are missing and why the created tree is incomplete (from the callers point of view) - debugging ensues and wastes some time. If you throw an ArgumentException instead stating the the parent can't be found - it's immediately clear what's wrong.

-
This is probably a bug: If you call Parent on an empty tree passing in an item with ParentKey == null then you will get the current tree as the parent even though the tree contains no items at all (the first two if blocks should probably be swapped around).

-
In Parent the the local variable is called child even through you're looking for the parent.

-
In generally if I call a data structure to remove an item the I do expect the data structure to be modified (i.e. the item removed) if the call is valid but not the item to be changed. RemoveItemAndChildren changes the passed in item which violates that expectation which is probably not a good thing.

-
The code in the Remove methods will throw a NullReferenceException when being passed in an invalid item since Parent may return null. It would be better to detect this and ignore the Remove call (the general expectation usually is that Remove just does nothing if being called with an item which is not in the structure).

-
Documentation for the public interface of the Tree class is missing (see point 2 above plus your comment - documentation would have made clear what the exact purpose of the method was)

Context

StackExchange Code Review Q#115545, answer score: 2

Revisions (0)

No revisions yet.