13

Assume I have the following classes in the same assembly

public class ParentClass : IDisposable
{
  public ChildClass Child
  {
    get { return _child; }
  }
}   

public class ChildClass 
{
   public ParentClass Parent
   {
     get { return _parent; }
     set { _parent= value; }
   }

   public ChildClass (ParentClass parent)
   {
     Parent= parent;
   }

}

Correct me if I am wrong but this IS bad design. Will this lead to a memory leak or some other unforseen issues later on? Apparently the garbage collector is capable of handling such kind of circular references.

Edit

What if the two classes end up getting used like this in some other class?

ParentClass objP = new ParentClass ();
ChildClass objC =new ChildClass(objP);
objP.Child = objC;

Thoughts please ....

Community
  • 1
  • 1
user20358
  • 14,182
  • 36
  • 114
  • 186
  • 9
    In a hierarchical tree data structure (e.g. `ProductGroup` class with `ParentProductGroup` property), this is totally valid in my opinion. – Uwe Keim Mar 07 '12 at 15:25
  • 2
    mmm.. I don't know how "bad" is it.. this is the definition of a double linked list.. and GC take care of it.. – gbianchi Mar 07 '12 at 15:28
  • 2
    O/RM tools generate code like this all the time. – Steven Mar 07 '12 at 15:30
  • I would set the child property in the parent class in the constructor of the ChildClass rather than how you example shows... – Sam Holder Mar 07 '12 at 15:33
  • ChildClass has a ParentClass; and ParentClass has a ChildClass; but is there a missing 'is a' relationship that is omitted in the example? I realize they are stereotyped as distinct, but are they intended to have the same behavior, and actually derive from the same root? Also is it intended to allow a child to be it's own Parent, too? I am a big fan of leaf-branch or container-component characterizations over parent-child most of the time. – JustinC Mar 07 '12 at 20:58

4 Answers4

18

Don't worry about the garbage collector; it handles reference graphs with arbitrary topologies with ease. Worry about writing objects that lend themselves to creating bugs by making it easy to violate their invariants.

This is a questionable design not because it stresses the GC -- it does not -- but rather because it does not enforce the desired semantic invariant: that if X is the parent of Y, then Y must be the child of X.

It can be quite tricky to write classes that maintain consistent parent-child relationships. What we do on the Roslyn team is we actually build two trees. The "real" tree has only child references; no child knows its parent. We layer a "facade" tree on top of that which enforces the consistency of the parent-child relationship: when you ask a parent node for its child, it creates a facade on top of its real child and sets the parent of that facade object to be the true parent.

UPDATE: Commenter Brian asks for more details. Here's a sketch of how you might implement a "red" facade with child and parent references over a "green" tree that only contains child references. In this system it is impossible to make an inconsistent parent-child relationship, as you can see in the test code at the bottom.

(We call these "red" and "green" trees because when drawing the data structure on the whiteboard, those were the marker colours we used.)

using System;

interface IValue { string Value { get; } }
interface IParent : IValue { IChild Child { get; } }
interface IChild : IValue { IParent Parent { get; } }

abstract class HasValue : IValue
{
    private string value;
    public HasValue(string value)
    {
        this.value = value;
    }
    public string Value { get { return value; } }
}

sealed class GreenChild : HasValue
{
    public GreenChild(string value) : base(value) {}
}

sealed class GreenParent : HasValue
{
    private GreenChild child;
    public GreenChild Child { get { return child; } }
    public GreenParent(string value, GreenChild child) : base(value)
    { 
         this.child = child; 
    }

    public IParent MakeFacade() { return new RedParent(this); }

    private sealed class RedParent : IParent
    {
        private GreenParent greenParent;
        private RedChild redChild;
        public RedParent(GreenParent parent) 
        { 
            this.greenParent = parent; 
            this.redChild = new RedChild(this);
        }
        public IChild Child { get { return redChild; } }
        public string Value { get { return greenParent.Value; } }

        private sealed class RedChild : IChild
        {
            private RedParent redParent;
            public RedChild(RedParent redParent)
            {
                this.redParent = redParent;
            }
            public IParent Parent { get { return redParent; } }
            public string Value 
            { 
                get 
                { 
                    return redParent.greenParent.Child.Value; 
                } 
            }
        }
    }
}
class P
{
    public static void Main()
    {
        var greenChild1 = new GreenChild("child1");
        var greenParent1 = new GreenParent("parent1", greenChild1);
        var greenParent2 = new GreenParent("parent2", greenChild1);

        var redParent1 = greenParent1.MakeFacade();
        var redParent2 = greenParent2.MakeFacade();

        Console.WriteLine(redParent1.Value); // parent1
        Console.WriteLine(redParent1.Child.Parent.Value); // parent1 !
        Console.WriteLine(redParent2.Value); // parent2
        Console.WriteLine(redParent2.Child.Parent.Value); // parent2 !

        // See how that goes? RedParent1 and RedParent2 disagree on what the
        // parent of greenChild1 is, **but they are self-consistent**. They
        // always report that the parent of their child is themselves.
    }
}
Eric Lippert
  • 647,829
  • 179
  • 1,238
  • 2,067
  • Can you please go into more detail about how the "facade" tree is used/generated? – Brian Mar 07 '12 at 16:25
  • This is a very interesting model. I have done something similar in the past using 'occurrence' classes that reference the real objects, but I hadn't thought about the 'facade' idea. Does this relationship-modeling style have a name and do you know of any further reference material? – Andrew Hanlon Mar 08 '12 at 14:06
  • @ach: We call it "red-green trees", but we just made that name up. I don't know of any further documentation on this particular pattern. – Eric Lippert Mar 08 '12 at 14:30
  • @EricLippert Or maybe they are fans of Canadian television http://en.wikipedia.org/wiki/The_Red_Green_Show – asawyer Mar 08 '12 at 14:40
  • @asawyer: The C# team is chock full of Canadians, myself among them, and so of course that has come up. However, the person who coined the term is not Canadian and just happened to have red and green whiteboard markers at the time. It could have just as easily been "black and blue trees", which would have rather a different connotation! – Eric Lippert Mar 08 '12 at 14:43
  • Thank you for the follow-up, I will have to do some testing. Interesting to know that my fellow Canadians are well represented in Redmond. – Andrew Hanlon Mar 08 '12 at 14:58
  • @ach: We hire a lot of Waterloo co-op students and graduates. – Eric Lippert Mar 08 '12 at 15:27
  • @EricLippert is there a reason for not having `Green{Parent,Child}` not implement the respective interfaces in the above example, or are they there strictly for allowing you to hide the Red implementation classes? – Matt Enright Mar 17 '12 at 19:25
  • @MattEnright: This is just a sketch. In our actual implementation we do it the other way -- the "green" tree is a private implementation detail of the "red" tree, and the red tree is the public surface area. – Eric Lippert Mar 17 '12 at 22:58
  • @EricLippert: In the real-life case, do you take advantage of the fact that both trees implement a common interface and are thus interchangeable, or is the abstraction simply to hide the implementation? Apologies for being unclear! – Matt Enright Mar 19 '12 at 19:10
  • @MattEnright: In the real-life case we don't have any interfaces at all; everything is done with classes. – Eric Lippert Mar 19 '12 at 20:07
11

It's neither particularly bad design nor a problem from a technical standpoint. Class instances (objects) are reference types, that is, _parent and _child only hold a reference to the respective object, not the object itself. So you don't cause some infinite data structure.

As you posted yourself, the garbage collector is able to handle cyclic references, mainly because it doesn't use reference counting.

The only "issue" with this kind of structure typically only is that you often want to keep both ends of the relation synchronized, ie. given Child c and Parent p then p.Child == c if and only if c.Parent == p. You'll need to decide how to handle this in the best way. For example, if you have a method Parent.AddChild(Child c) this could not only set Parent.Child to c, but also Child.Parent to the parent. But what if Child.Parent is assigned directly? Those are some question you might have to deal with.

Sam Holder
  • 32,535
  • 13
  • 101
  • 181
Andre Loker
  • 8,368
  • 1
  • 23
  • 36
  • thanks for the reply.. so even in the way I have demonstrated it in my updated post it should not be a problem right? – user20358 Mar 07 '12 at 15:35
  • No, but as I wrote you might want to ensure that both ends of the relationship are synchronized. – Andre Loker Mar 07 '12 at 15:37
  • what kind of issues can happen if properties become out of sync? How would they go out of sync? would there be any impact to this kind of circular reference if there were not to be a parent child relationship between the two classes? As in if these two classes were doing different things and say one class used the other for performing certain utility tasks. – user20358 Mar 07 '12 at 15:59
  • Maybe I put it confusingly. There's nothing technically wrong if Child.Parent != Parent.Child. It won't affect performance or anything else of your app. It's just weird from a logical standpoint, it's just what you'd expect. But maybe this is allowed in your domain, so everything's fine. – Andre Loker Mar 07 '12 at 17:41
  • (cont'd) Re: one class using the other. Admittedly true bidirectional one-to-one relations are not that common and it *does* smell suspicious if the two classes mutually need each other. Somehow their functionality seems to be entangled. If you have a more specific example I can tell you what I think about it. Sometimes it helps to reduce the responsibility of the classes or at least the size of the interface to decouple classes (see e.g. [Interface Segregation Principle](http://en.wikipedia.org/wiki/Interface_segregation_principle)). – Andre Loker Mar 07 '12 at 17:45
  • (cont'd): I'm not saying bidirectional one-to-one relations shouldn't exist. But it is often advisable to keep the coupling *between* classes low. – Andre Loker Mar 07 '12 at 17:47
3

This is totaly fine IMO.
MS as an example does this in most UI Controls/Elements e.g. a Form does have a Controls Collection and each Control in that Collection has a ParentForm Property.

Christoph Fink
  • 22,727
  • 9
  • 68
  • 113
  • “Even MS does this” doesn't always mean you should do it too. That applies especially for some of the older parts of the Framework. – svick Mar 07 '12 at 15:31
  • That is indeed true, just wanted to give a example where this is used "in a larger scale"... – Christoph Fink Mar 07 '12 at 15:33
  • So if these were two classes that did not have a parent child relationship then it would mean some sort of a code smell now would it? :) – user20358 Mar 07 '12 at 15:54
2

This is ok I think and is useful if you need to be able to navigate the relationship in both directions.

As you have pointed out the GC can handle it, so why should it be an issue? If it provides the functionality you need, then it should be fine.

You should be wary of issues with the properties becoming out of sync, assuming that this is going to be an issue with our code.

Sam Holder
  • 32,535
  • 13
  • 101
  • 181
  • thanks.. updated my question. so even in the way I have demonstrated it being handled it should not be a problem right? – user20358 Mar 07 '12 at 15:34
  • no but I would set the child property in the parent class in the constructor of the ChildClass rather than how you example shows, and would probably add sdome checking code to ensure that the relationships stay in sync... – Sam Holder Mar 07 '12 at 15:38
  • what kind of issues can happen if properties become out of sync? How would they go out of sync? – user20358 Mar 07 '12 at 15:56
  • Can this kind of reference be safely used in a non parent child relationship between classes? – user20358 Mar 07 '12 at 15:56
  • 1
    what is to stop someone setting Child c1. Parent p1. Child c2. Parent p2. c1.Parent=p1. p1.Child=c2. c2.Parent=p2 Now they are out of sync as the parent of the child of p1 is not p1. And yes it can be used to represent any relationship which you wish to navigate in both directions. – Sam Holder Mar 07 '12 at 16:04
  • yea that makes sense. Sorry to drag this some more but it seems like if two classes are dependent on each other then there is something that needs to be pulled out from both those classes and put into a third class.. so both of them can use it ..and that'd be a more convenient way to manage things? ...would you agree with my thinking? – user20358 Mar 07 '12 at 16:27