Is it bad practice to have a class that requires a reference to the instantiating object?

Question

I saw this in someone's code and thought wow, that's an elegant way to solve this particular problem, but it probably violates good OO principles in an epic way.

In the constructor for a set of classes that are all derived from a common base class, he requires a reference to the instancing class to be passed. For example,
Foo Foo_i = new(this);

Then later on Foo would call methods in the instancing class to get information about itself and the other objects contained by the instancing class.

On the one hand, this simplifies a TON of code that models a 5-layer tree structure in hardware (agents plugged into ports on multiple switches, etc). On the other hand, these objects are pretty tightly coupled to each other in a way that seems pretty wrong, but I don't know enough about OOA&D to put my finger on it.

So, is this okay? Or is this the OO equivalent to a goto statement?

Answer 1

I try and avoid this if I can just from an information hiding point of view. The less information a class has or needs the easier it is to test and verify. That being said, it does lead to more elegant solutions in some cases so if not doing it is horribly convoluted involving an awful lot of parameter passing then by all means go for it.

Java for example uses this a lot with inner classes:

public class Outer {
  private class Inner {
    public Inner() {
      // has access to the members of Outer for the instance that instantiated it
    }
  }
}

Answer 2

In Java, I remember avoiding this once by subclassing certain Listeners and Adapters in my controller and adding those listeners and adapters to my subclasses.

In other words my controller was

class p {

  private member x

  private methods

  private class q {
    // methods referencing p's private members and methods
  }

  x.setListener(new q());

}

I think this is more loosely coupled, but I would also like some confirmation.

Answer 3

This design pattern can make a lot of sense in some situations. For example, iterators are always associated with a specific collection, so it makes sense for the iterator's constructor to require a collection.

You didn't provide a concrete example, but if the class reminds you of goto, it probably is a bad idea.

You said the new object must interrogate the instantiating object for information. Perhaps the class makes too many assumptions about its environment? If those assumptions complicate unit testing, debugging, or (non-hypothetical) code reuse, then you should consider refactoring.

But if the design saves developer time overall and you don't expect an unmaintainable beast in two years' time, the practice is completely acceptable from a practical standpoint.

Answer 4

This sounds like it could be violating the Law of Demeter, depending on how much Foo needs to know to fish around in the instancing class. Objects should preferably be loosely coupled. You'd rather not have one class need to know a lot about the structure of another class. One example I've heard a few times is that you wouldn't hand your wallet over to a store clerk and let him fish around inside. Your wallet is your business, and you'll find what you need to give the clerk and hand it over yourself. You can reorganize your wallet and nobody will be the wiser. Looser coupling makes testing easier. Foo should ideally be testable without needing to maintain a complex context.

Answer 5

You shoud try to avoid mutual references (especially when implemeting containment) but oftentimes they are impossible to avoid. I.e. parent child relationship - children often need to know the parent and notify it if some events happen. If you really need to do that - opt for interfaces (or abstract classes in case of C++). So you instancing class should implement some interface, and the instanciated class should know it only as interface - this will sigificantly reduce coupling. In some respect this approach is similar to nested listener class as it exposes only part of the class, but it is easier to maintain. Here is little C# example:

interface IParent
{
    //some methods here
}

class Child
{
    // child will know parent (instancing class) as interface only
    private readonly IParent parent_;
    public Child(IParent parent)
    {
        parent_ = parent;
    }
}

class Parent : IParent
{
    // IParent implementation and other methods here
}