Hierarchical object and AutoFixture

Question

I have implemented a class for storing tags, the tag collection must be hierachical, and so my class is:

public class Tag
{
    public int Id { get; set; }
    public int Description { get; set; }
    public Tag ParentTag { get; set; }
    // … (methods for get children, add and remove children, etc.)
}

In this way root tags (user want to be able to have many separate trees) has no parent, while non-root tags must have a parent tag.

Is this a good way to implement hierarchy? I find Composite Pattern, but in my domain, all tags are simply tags, to the domain expert, there is no difference between parent and child tag.
Problem come using AutoFixture in test; when I need to create a simple tag, it raises this error:

Failure: Ploeh.AutoFixture.ObjectCreationException: AutoFixture was unable to create an instance of type Ploeh.AutoFixture.Kernel.SeededRequest because the traversed object graph contains a circular reference.

Edit: i read Creating recursive tree with AutoFixture but it's different case: i have only one class, not 2 and i don't want autofixture to create a tree but only a single node

sorry, i edit class changing ParentTag type, the int version was a mistake. Desc property was for description, edited too — gt.guybrush, Jun 20 '14 at 12:09

score 6 · Accepted Answer · edited Jun 20 '20 at 09:12

Is this a good way to implement hierarchy?

I see three problems with it, one minor, one a little more serious, and one obviously problematic in your concrete situation.

Potential issues:

1. Let's start with the minor issue, which is about the relationship between properties' names and their types. I would recommend that a property named ParentTag should be of the Tag type itself. The fact that you declared it as int (like you did for Id) suggests that you should call the property ParentTagId instead… or that you change the property's type to Tag.

2. Now to the more serious issue. I take it that Desc points to an immediate child tag. (In case that one tag can have more than one child tag, you have obviously chosen the wrong type for this property. You'd need some kind of collection. But that's yet another problem.)

Storing both parent and children links can easily lead to inconsistencies if you're not paying good attention. It might therefore be better not to have bi-directional links for each tag, but to only store links going in one direction.

That however will complicate traversing the hierarchy in the opposite direction. One way to solve this problem would be to store only child links; if you then wanted to find the parent tag of T, you'd first find T by recursively traversing the hierarchy starting at the root tag and continuously keep track of the "path" you're taking; the parent will then be the penultimate tag in the path.

3. Now to the most immediate issue. The exception hints at it:

Ploeh.AutoFixture.ObjectCreationException […] because the traversed object graph contains a circular reference.

With your current implementation of Tag, it's possible to build tag hierarchies that contain cycles. I assume that you don't want that.

For example, tag C can have P as its parent tag, although P is already a child tag of C. Therefore, if you started following the ParentTag chain starting at C, you would first get to P and then eventually arrive back at C, and if you kept going, you would find yourself caught in an infinite loop.

I don't know AutoFixture, but it seems likely that it cannot deal with your concrete tag hierarchy for similar reasons.

You should make your tag hierarchies directed acyclic graphs (DAGs) — the "acyclic" is the important bit here. However, with your current Tag class, you can build any directed graph; it does not guarantee that there won't be any cycles.

Ways to prevent cyclic tag hierarchies:

1. Implement a check for cycles in the ParentTag setter:

public Tag ParentTag
{
    …
    set
    {
        if (!IsOrIsAncestorOf(value))
        {
            parentTag = value;
        }
        else
        {
            throw new ArgumentException("ParentTag", "would cause a cycle");
        }
    }
}
private Tag parentTag;

private bool IsOrIsAncestorOf(Tag other)
{
    return this == other || IsOrIsAncestorOf(other.Parent));
    //     ^^^^^^^^^^^^^    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    //          Is   …   Or    …    IsAncestorOf
}

2. Even simpler, make ParentTag readonly, which forces you to set it in the constructor. This will automatically make it impossible to build cyclic tag hierarchies — try it, if you don't believe it:

public Tag(Tag parentTag)
{
    this.parentTag = parentTag;
}

private readonly Tag parentTag;

public Tag ParentTag
{
    get
    {
        return parentTag;
    }
}

I would recommend the second solution.

and more, adding a parameter to constructor i was no more able to create a fake with Moq — gt.guybrush, Jun 20 '14 at 12:33
and, as you suggested, the second solution: simply and concise — gt.guybrush, Jun 20 '14 at 14:32
you write 'One way to solve this problem would be to store only child links' but in this way how can i move a Tag? even now i have to remove readonly attribute from private filed to be able to change it in a Move() methos — gt.guybrush, Jun 20 '14 at 14:46
Different question. I don't think we can solve this in the comment section. But basically, you would move a tag by rebuilding the hierarchy. This sounds worse than it is; after all you can "recycle" unaffected parts. — stakx - no longer contributing, Jun 20 '14 at 15:50
ok, i have created new question here: http://stackoverflow.com/questions/24333693/c-sharp-should-i-have-parent-o-listofchild-property-in-hierarcical-object — gt.guybrush, Jun 20 '14 at 18:42
@stakx Nice answer. One question though: how would the class with the `readonly` option *automatically* prevent any cycles? Is it just in this particular case or generally, because it is still possible to create a cycle? — keenthinker, Jun 22 '14 at 16:18
@pasty: It is because an object cannot be referenced until it has been fully constructed, i.e. until `new` returns to the caller. Therefore two objects cannot reference each other if that reference is only passed in via the constructor. (Let's say you have object B already. This means you passed a reference to another object A into B's constructor. Which means that you could not have passed B to A's constructor, because A must have been there first; otherwise you couldn't pass it to B's constructor.) If you don't understand it, just give it a go. — stakx - no longer contributing, Jun 22 '14 at 16:31
@pasty: Not sure what you're saying, or asking. I have never used AutoFixture, so cannot comment on issues specific to it. I was merely trying to point out that constructor injection can be used to prevent cyclic references. Note that in your example, the object referenced by the variable `tag3` does not reference the object `tag2` refers to in the end; the `tag3` object references `null`, because it's not the variable `tag2` that gets captured by the `Tag` constructor, but its value. — stakx - no longer contributing, Jun 22 '14 at 21:02
@stakx: Got it, thanks. I wrote a simple app and examined the output. — keenthinker, Jun 22 '14 at 21:59
iam implementing it now with NHibernate but found a problem: how can remove a tag from parent Child list if i can't edit parent property? — gt.guybrush, Aug 14 '14 at 06:34
If it is the parent that has the `Child` list, then edit that list of the parent, not the `Parent` property of the child. (That being said, if this is really a new question, then please also ask a new question on SO, instead of starting a follow-up chat session here.) — stakx - no longer contributing, Aug 14 '14 at 06:53

Hierarchical object and AutoFixture

1 Answers1

Linked