2
\$\begingroup\$

Is there an existing class in .Net for representing and querying hierarchical data; i.e. providing methods which allow you to test whether one item is an ancestor or descendant of another, or to provide a way to fetch all ancestors/descendants recursively from a given object?

Failing that, I've created a very basic example to illustrate the sort of thing I'm after... I've not done any optimisation as I'm hoping this is a common enough requirement that there's already something OOTB or a package I could use instead of rolling my own; though so far I haven't found one. I'd welcome feedback on this approach in case I've missed any good tricks which may improve this approach. Also, I realise this current approach allows the dictionary's key to be a mutable type, which may lead to issues with some use cases, so if there's a way to enforce the type to be immutable that would be helpful.

public class HierarchicalNode<T> where T : IComparable
{
    public T Value {get;}
    public HierarchicalNode<T> Parent {get;}
    public List<HierarchicalNode<T>> Children {get;}
    public HierarchicalNode(T value, HierarchicalNode<T> parent = null) 
    {
        this.Children = new List<HierarchicalNode<T>>();
        this.Value = value;
        this.Parent = parent;
        parent?.Children.Add(this);
    }
    public bool IsAncestorOf(HierarchicalNode<T> item, bool includeSelf = true) 
    {
        return item != null 
            && (
                includeSelf && this.Value.CompareTo(item.Value) == 0
                || IsAncestorOf(item.Parent)
            )
        ;
    }
    public bool IsDescendantOf(HierarchicalNode<T> item, bool includeSelf = true) 
    {
        return item != null 
            && (
                includeSelf && this.Value.CompareTo(item.Value) == 0
                || (this.Parent?.IsDescendantOf(item) ?? false)
            )
        ;
    }
    public bool IsRoot()
    {
        return this.Parent == null;
    }
    public IEnumerable<T> GetDescendants(bool includeSelf = false) 
    {
        if (includeSelf)
        {
            yield return Value;
        }
        foreach (var child in Children)
        {
            yield return child.Value;
            foreach (var descendant in child.GetDescendants()) 
            {
                yield return descendant;
            }
        }
    }
    public IEnumerable<T> GetAncestors(bool includeSelf = false) 
    {
        if (includeSelf)
        {
            yield return Value;
        }
        foreach (var ancestor in this?.Parent?.GetAncestors(true) ?? Enumerable.Empty<T>())
        {
            yield return ancestor;
        }
    }
}

# Note: I'm including a dictionary so there's an easy way to get all values without having to traverse the graph / so we can represent islands of disconnected data; i.e. if there are several root nodes.
public class HierarchicalDictionary<Key, T> 
    where T : IComparable
    where Key: class 
{
    private Dictionary<Key, HierarchicalNode<T>> dict = new Dictionary<Key, HierarchicalNode<T>>();
    private Func<T, Key> getKey;
    Func<T, Key> getParentKey;
    public HierarchicalDictionary(Func<T, Key> getKey, Func<T, Key> getParentKey) 
    {
        this.getKey = getKey;
        this.getParentKey = getParentKey;
    }
    public void Add(T item) {
        if (item == null) throw new ArgumentNullException(nameof(item));
        var key = getKey(item);
        var parentKey = getParentKey(item);
        if (parentKey != null && !dict.Keys.Contains(parentKey)) {
            throw new InvalidOperationException($"Parent must exist before child can be added. Key [{key}]. ParentKey [{parentKey}]");
        }
        if (dict.Keys.Contains(key)) {
            throw new InvalidOperationException($"An object with the key [{key}] already exists");
        }
        var parent = parentKey == null ? null : dict[parentKey];
        var node = new HierarchicalNode<T>(item, parent);
        dict.Add(key, node);
    }
    public void AddRange(IEnumerable<T> item) 
    {
        foreach (var i in item)
        {
            this.Add(i);
        }
    }
    public bool IsAncestorOfByKey(Key a, Key b, bool includeSelf = true)
    {
        var objA = dict[a];
        var objB = dict[b];
        return objA.IsAncestorOf(objB);
    }
    public bool IsDescendantOfByKey(Key a, Key b, bool includeSelf = true)
    {
        var objA = dict[a];
        var objB = dict[b];
        return objA.IsDescendantOf(objB);
    }
    public IEnumerable<T> GetDescendantsOfByKey(Key key)
    {
        return dict[key].GetDescendants();
    }
    public IEnumerable<T> GetAncestorsOfByKey(Key key)
    {
        return dict[key].GetAncestors();
    }
}

Example Implementation: DotNet Fiddle

void Main()
{
    var employees = new Employee[]
    {
        new ("[email protected]","Brie Emmental", null),
        new ("[email protected]","Adrian Steward","[email protected]"),
        new ("[email protected]","Max Power", "[email protected]"),
        new ("[email protected]","Robert Ott","[email protected]"),
        new ("[email protected]","Anne Droid","[email protected]"),
        new ("[email protected]","Simon Borg","[email protected]"),
        new ("[email protected]","Person A",null),
        new ("[email protected]","Person B","[email protected]"),
        new ("[email protected]","Person C","[email protected]"),
        new ("[email protected]","Person X","[email protected]"),
        new ("[email protected]","Person Y","[email protected]"),
        new ("[email protected]","Person Z","[email protected]")
    };
    var roster = new HierarchicalDictionary<string, Employee>(x => x.Id, x => x.ManagerId);
    roster.AddRange(employees);
    foreach (var a in employees) 
    {
        foreach (var b in employees) 
        {
            var relationship = roster.IsAncestorOfByKey(a.Id, b.Id) ? "manages" : "does not manage";
            Console.WriteLine($"[{a.Name}] {relationship} [{b.Name}]");
            relationship = roster.IsDescendantOfByKey(a.Id, b.Id) ? "reports to" : "does not report to";
            Console.WriteLine($"[{a.Name}] {relationship} [{b.Name}]");
        }
    }
    Console.WriteLine();
    foreach (var descendant in roster.GetDescendantsOfByKey("[email protected]")) 
    {
        Console.WriteLine($"Employees Of Max: [{descendant.Name}]");
    }
    Console.WriteLine();
    foreach (var descendant in roster.GetAncestorsOfByKey("[email protected]")) 
    {
        Console.WriteLine($"Managers Of Robert: [{descendant.Name}]");
    }
    Console.WriteLine("End of demo");
}

public class Employee: IComparable
{
    public string Id {get;}
    public string Name {get;}
    public string ManagerId {get;}
    public Employee (string id, string name, string managerId)
    {
        Id = id;
        Name = name;
        ManagerId = managerId;
    }
    public int CompareTo(object obj) {
        return this.Id.CompareTo((obj as Employee)?.Id);
    }
}
\$\endgroup\$
3
  • 4
    \$\begingroup\$ The title sounds complete off topic. So what the code, you want to be reviewed, is doing? \$\endgroup\$
    – convert
    Commented Jun 29, 2023 at 12:05
  • 1
    \$\begingroup\$ I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Please check that I haven't misrepresented your code, and correct it if I have. \$\endgroup\$ Commented Jun 29, 2023 at 21:24
  • \$\begingroup\$ Thank-you both for pointing that out / updating the title. Agree that my original question wasn't really appropriate for this site; thanks. \$\endgroup\$
    – JohnLBevan
    Commented Jul 1, 2023 at 6:13

3 Answers 3

4
\$\begingroup\$

This is a nice implementation. Its written in a way that it is easy to read and understand. That being said, let's face some issues :-)

HierarchicalDictionary<Key, T>

AddRange() here I would expect the parameter-name to be in the plural form because you add a range of items and the type of the parameter is IEnumerable. For my pesonal taste I would use T instead of var as well.

public void AddRange(IEnumerable<T> items) 
{
    foreach (T item in items)
    {
        this.Add(item);
    }
}

Add()
Instead of checking if a key exists and later on accessing the key you should use TryGetValue() because using dict[key] is just doing this as well and hence is called twice. Checking if the passed key exists should be done after the call to getKey() to return early. No need to do anything else if thekey is in the dictionary.

    public void Add(T item)
    {
        if (item == null) throw new ArgumentNullException(nameof(item));
        var key = getKey(item);
        if (dict.Keys.Contains(key))
        {
            throw new InvalidOperationException($"An object with the key [{key}] already exists");
        }

        var parentKey = getParentKey(item);
        HierarchicalNode<T> parent = null;
        if (parentKey != null && !dict.TryGetValue(parentKey, out parent)) //!dict.Keys.Contains(parentKey))
        {
            throw new InvalidOperationException($"Parent must exist before child can be added. Key [{key}]. ParentKey [{parentKey}]");
        }

        var node = new HierarchicalNode<T>(item, parent);
        dict.Add(key, node);
    }

GetDescendantsOfByKey() and GetAncestorsOfByKey() here you should check if the passed key exists.

    public IEnumerable<T> GetDescendantsOfByKey(Key key)
    {
        if (!dict.TryGetValue(key, out var node))
        {
            throw new KeyNotFoundException($"An object with the key[{ key }] doesn't exists");
        }
        return node.GetDescendants();
    }
    public IEnumerable<T> GetAncestorsOfByKey(Key key)
    {
        if (!dict.TryGetValue(key, out var node))
        {
            throw new KeyNotFoundException($"An object with the key[{ key }] doesn't exists");
        }
        return node.GetAncestors();
    }

The Func<T, Key> getParentKey; should have an access modifier as well.

\$\endgroup\$
4
  • \$\begingroup\$ Nice one, thank-you for the feedback. Regarding var vs T is there any argument behind favouring T in this scenario, or is that solely a preference? Beyond that (on which I currently don't currently have a preference either way), I fully agree with all your suggestions. Thank-you again. \$\endgroup\$
    – JohnLBevan
    Commented Jun 29, 2023 at 6:28
  • \$\begingroup\$ I would argue with your statement that it is easy to read and understand. I have to read multiple times the IsDescendantOf and IsAncestorOf to understand its recursive nature \$\endgroup\$ Commented Jun 29, 2023 at 6:34
  • 1
    \$\begingroup\$ @PeterCsala I have meant the overall. For that two methods it would be better to place the recursive calls on a separate line. \$\endgroup\$
    – Heslacher
    Commented Jun 29, 2023 at 6:59
  • \$\begingroup\$ Regarding var vs T, no there isn't any argument behind, but IMO it is more clear. Just personell prference I guess. \$\endgroup\$
    – Heslacher
    Commented Jun 29, 2023 at 7:00
3
\$\begingroup\$

Would you not be able to do the same with LINQ, SelectMany, and then asking if an a reference comparer to all results from your select many?

The only issue I can see with this is, you either need to make it recursive, if you don't know the depth?

otherwise? that should do it? If performance is an issue, than insert hashkeys on instances?

And you just have lists of the same class on the instance of said class?

What I mean is, the "linked set" mechanism could just be put on the classes themselves, as a responsibility they should manage, and then you don't need a "new" List type?

Expanding on my "answer" that is partially a question, sorry!

    public class Employee{
    
           public Employee? ParentEmployee {get;set;}
           public List<Employee>? ChildrenEmployees {get;set;}
           //public HashSet<Employee>? children {get;set;}
     
  
    public Employee GetTopParent() {
          //Get the "node" with no parent.
      }
    
      public List<Employee> GetChildrenRecursively(){
          //Get all childrens, childrens, children etc. until the children lists are null. or count == 0 or whatever you choose.
      }
    
      public Employee FindEmployee(Employee input)
      {
         Employee topParent = GetTopParent();
         return topParent.FindTargetInChildren(input);
      }
     
      public Employee FindTargetInChildren(Employee input){
         //search recursively downwards
      }
}

If you changed the Employee reference to an Interface, and made the interface contain HashSet value instead, you could likely gain performance. Those values would technically not need to be the object itself? possible making the search faster. You might even be able to DI different methods into classes?

or simply say something like:

Employee employee = getEmployee(); // from some context?

List<Employee> parents = employee.SelectMany(x => x.GetAllParentsRecursively());
List<Employee> children = employee.SelectMany(x => x.GetAllChildrenRecursively());

Then you have all objects. You can also choose to ask for a property, to be the "target" etc. Then the logic of traversing the "linked set" of instances, is built into the instances.

My main concern is, is it easy to test? and recursive methods tend to be hard to read, from a purely readability standpoint. So might not be an awesome idea. But overall, it should be more flexible to change.

Say if you have a list of Employees, but you need to include Manager class, or Peon class that are linked to Employees, that becomes harder to build a traverse for in your case. Because you have determined that a specific type is required for the search.

If you build it by interface, you can implement way more changes, without breaking everything, and do it decoupled so there are no direct dependencies. right? That should make this much more resistent to "Uh, I want to be able to do x, not just y" ...

You seem happy to search by a full object, but what if you wanted to search by partial values? or specific properties. This method would be easier to accommodate changes for, I think?

You could also implement an IEmployee interface, and have types like "Manager : IEmployee" and "Slave : IEmployee, and still do a full search across the Top node, to check if the same slave is located to a different manager, for example :D

That would be harder to accomplish with the original suggestion right?

\$\endgroup\$
4
  • \$\begingroup\$ I'm not sure I follow... When you say the "linked set" mechanism could just be put on the classes themselves do you mean coding the logic into the Employee class? For me, hierarchical data is a common pattern (e.g. nested security groups, file system directory structures, dependency graphs, family trees, etc). The logic for dealing with the hierarchical aspects is generally the same (you want to check whether there's a "direct line of inheritance" between two items, or you want to recurively list everything that's in that line from an item). i.e. a good candidate for encapsulation/generics \$\endgroup\$
    – JohnLBevan
    Commented Jun 29, 2023 at 11:14
  • \$\begingroup\$ RE Linq/SelectMany, I agree there's other ways to code this logic... Is there a reason you'd suggest that method over the current implementation; i.e. what are the benefits to that approach over the current solution? \$\endgroup\$
    – JohnLBevan
    Commented Jun 29, 2023 at 11:16
  • 1
    \$\begingroup\$ @JohnLBevan please review my changes, I tried to be clearer, think I might have failed :D \$\endgroup\$ Commented Jun 29, 2023 at 11:51
  • \$\begingroup\$ Thanks Morten; your suggestion of using interfaces for Employee makes sense. The Employee example was a demo of how I'd expect the generic hierarchical dictionary to be used, rather than the code I'm concerned about. The hierarchical dictionary is the bit I care about; as I've had to write similar logic for nagivating parent/child graphs a number of times / figured there should be something I could reuse. You're right though that I could put an interface on that then DI it into Employee, so I can swap it for an alternative for other scenarios (e.g. for one allowing circular dependencies). \$\endgroup\$
    – JohnLBevan
    Commented Jul 1, 2023 at 6:29
0
\$\begingroup\$

Since writing this I realised an obvious improvement...

I'd written functions IsAncestorOf and IsDecendentOf; but both of these essentially do the same thing; just switching the arguments. So to reduce maintenance, IsDecendentOf could be rewritten as:

public bool IsDescendantOf(HierarchicalNode<T> item, bool includeSelf = true) 
{
    return item != null 
        && item.IsAncestorOf(this);
    ;
}
\$\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.