Is passing(in constructor) a pointer to class that contains it a bad design and if so what is the solution

Question

often I encounter code like

/*initializer list of some class*/:m_member(some_param,/* --> */ *this)

Reason why this is done is so that m_member can call member functions from the class that contains it... aka

//code in class that is m_member instance of

    m_parent->some_function();

I personally dislike it because I consider it pathetic design("dear child do you know what are you doing to your class encapsulation"), but I would like to know is in general this behavior bad, and if so how to avoid this kind of design.

EDIT: please dont focus on this in initalizer list, lets say it is in ctor body.

Answer 1

It can be disastrous, since your parent is not constructed a the time of the reference-set. The following example will demonstrate this:

#include <iostream>
using namespace std;

struct TheParent;

struct TheChild
{
    TheChild(TheParent& parent);
    TheParent& myParent;
};

struct TheParent
{
    TheParent()
      : mychild(*this)
      , value(1)
    {
        cout << "TheParent::TheParent() : " << value << endl;
    }

    TheChild mychild;
    int value;
};

TheChild::TheChild(TheParent& parent)
   : myParent(parent)
{
    cout << "TheChild::TheChild() : " << myParent.value << endl;
};

int main()
{
    TheParent parent;
    return 0;
}

Produces the following output, clearly noting the indeterminate state of the parent object:

TheChild::TheChild() : 1606422622
TheParent::TheParent() : 1

Bottom line: don't do it this way. You would be better served to use a dynamic child allocation instead, but even this has caveats:

#include <iostream>
using namespace std;

struct TheParent;

struct TheChild
{
    TheChild(TheParent& parent);
    TheParent& myParent;
};

struct TheParent
{
    TheParent()
      : mychild(NULL)
      , value(1)
    {
        mychild = new TheChild(*this);
        cout << "TheParent::TheParent() : " << value << endl;
    }

    ~TheParent()
    {
        delete mychild;
    }

    TheChild* mychild;
    int value;
};

TheChild::TheChild(TheParent& parent)
   : myParent(parent)
{
    cout << "TheChild::TheChild() : " << myParent.value << endl;
};


int main()
{
    TheParent parent;
    return 0;
}

This give you what you're likely hoping for:

TheChild::TheChild() : 1
TheParent::TheParent() : 1

Note, however, even this has issues if TheParent is an intermediate class in an inheritance chain, and you're desiring to access potentially overridden virtual implementations of functions in derived classes that have yet to be constructed.

Again, bottom line, if you find yourself doing this, you may want to think about why you need to in the first place.

Answer 2

It is bad because it is unclear how complete the parent class is at the time m_member is constructed.

For example:

class Parent
{
   Parent()
   : m_member(this), m_other(foo)
   { }
};

class Member
{
    Member(Parent* parent)
    {
       std::cout << parent->m_other << std::endl; // What should this print?
    }
};

A slightly better approach if a parent pointer is needed is for Member to have a 'setParent' method called in the body of the constructor.

Answer 3

Like the vast majority of programming practices, it is impossible to say that it is bad in general (and if you do, you are a bad person and should be ashamed). I use this sometimes, but it is uncommon; however, it is not a thing I would try to purposefully avoid by changing my class design.

Note how I used "I" a lot in the above paragraph, a sure sign this is a highly subjective issue.

Answer 4

I see the language as a tool to implement the solution for a given problem. By design, C++ allows explicit uses of this and other OO languages don't. Thus, I look at language features as tools in my toolbox, and every so often there is a use to bring out one tool or another.

However, and that's where coding style and practice comes in, I should know what I'm doing. I should know how to use my tools, and I should know the implications of their use. There is a defined order in which C++ initializes a new object, and as long as I work with this then I'm good. Unfortunately, some times people get lucky; other times they create bugs that way. You need to know your tools and how to use them :-)

To answer your question with my personal opinion: I try to avoid this particular construct, but on occasion I had to use it. Even pondering a class re-design wouldn't have avoided that. And so I filed this occasion under, "Ah well, sometimes my design just can't be modeled in clean-clean straight OO, the dependencies between the classes are too tight and performance matters too much."