I have a contextual object that has several useful properties and methods. Other objects accept this contextual object in their constructors. However, the contextual object can have these other objects as public properties. See below:
public class Foo {
private IContext Context { get; private set; }
public Foo( IContext context ) {
Context = context;
}
}
public class SomeContext : IContext {
public Foo SomeProperty { get; set; }
/*
Other useful properties and methods as defined in IContext
*/
public SomeContext () {
SomeProperty = new Foo( this );
}
}
Now I can do something useful in a method on Foo:
public void FooIt() {
IUseful bar = this.Context.GetUsefulInterface();
bar.DoUsefulThing();
}
However, it can lead to some really weird stuff. Consider these methods on Foo:
public void DoSomething() {
/* useful stuff */
}
public void DoSomethingElse() {
this.Context.SomeProperty.DoSomething(); // could just call this.DoSomething();
this.Context.SomeProperty.DoSomethingElse(); // stack overflow!
}
Is this considered a bad design / code smell? The reasons for the contextual object are somewhat involved, and I'd like to frame the question more towards the cyclical reference. If considered bad, what are some approaches to breaking this cyclical relationship?
Without domain knowledge, it is hard to know where to start when trying to design out your cyclic dependency, so I'll just generalize.
Cyclic references are bad when you aren't expressing a hierarchical relationship, in which it is required or useful to get back to a parent. This is because it promotes strong coupling between the types in the cycle, it is difficult to construct/delete properly, and is prone to bugs when traversing.
However, when you have a hierarchical relationship, and it is required/useful to traverse back up the hierarchy, then it makes perfect sense to have cyclic references.
One thing you may want to avoid is trying to call too deep into your properties at one time. This increases your coupling, and can easily lead to bugs like the stack overflow exception you have found.
// Reaches too far. Makes this depend on the interface of SomeProperty
this.Context.SomeProperty.DoSomething();
// ...
// Not reaching too far. Only depends on Context.
// This might forward to SomeProperty.DoSomething()
this.Context.DoSomething();
Depending on how you fix it, this might also help you break the stack overflow.
See: http://en.wikipedia.org/wiki/Law_of_Demeter
Not bad, but definitely not ideal. My take on this is that you should not be creating a context for your object in your constructor where the context object takes the object being created. The context should be something external to the object you are creating.
However, there is no problem with creating an 'infinite' loop of sorts with your context object. The ASP.NET MVC ControllerContext does something very similar in ways; you just normally dont call it like so:
ControllerContext.Controller.ControllerContext.Controller // etc
Cyclic relationships are bad in non-managed environments because you need a weak reference to break cyclic relationships (or otherwise you'll inevitably leak the objects involved in the cycle). I can't think of a concrete reason to label them "bad" in a managed environment, though.
In fact, there are plenty of good reasons to have cyclic references. For instance, in a UI tree, parent UI elements know their children, and children know their parents too.
In common the cyclical references is smell of bad design. However there are several cases, where are cyclical references good way (in Winforms: Control.Controls vs. Control.Parent).
If you have cyclical references you should check for correct values. If you have parent-children relation, and the childr is removed from parent, the child shouldn't have reference to the parent and parent shouldn't have reference to child.
