5

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.

NoSenseEtAl
  • 28,205
  • 28
  • 128
  • 277
  • Presumably you mean `this` instead of `*this`? – Joseph Mansfield Dec 19 '12 at 17:48
  • Is _is_ bad, and you should get a compiler warning (at least) for using `this` in the initializer list. However, I see this a lot and I can't think of a good way to "fix" it in the simple sense (aside from a redesign). – Chad Dec 19 '12 at 17:49
  • it takes a ref, so it is semantically this. Btw lets ignore that it is in initalizer list... lets say it is in a ctor body. – NoSenseEtAl Dec 19 '12 at 17:51
  • In the constructor body itself, I don't particularly see a problem with it. – Chad Dec 19 '12 at 17:52
  • @Chad tnx... like I said I really dont like class depending on other class. – NoSenseEtAl Dec 19 '12 at 17:55
  • Classes will have interdependencies, even if you refactored this so that you were _only_ message passing between instances they would have some contract on the types of messages/content accepted. I do agree it's not "pretty", but in a lot of cases it is necessary. – Chad Dec 19 '12 at 17:57
  • There are cases where this is perfectly acceptable, especially if there is a tight coupling between the type of the data member and the type of the enclosing class (e.g. if parts of the enclosing class are refactored into another class for reuse). One must be careful, of course, but this is not necessarily bad. – James McNellis Dec 19 '12 at 18:23
  • Btw, it doesn't *necessarily* do anything horrible to the design. It might, but I can imagine a case where it doesn't. Suppose that class A holds a reference/pointer to an instance of (possibly-virtual) interface B, that it uses for some purpose. Class C implements interface B, and it does so in part via the use of a data member of type A holding a pointer to itself. The dependencies are "clean", there's no break in encapsulation. The problem is that it doesn't work if A uses the instance of B in A's constructor or destructor. – Steve Jessop Dec 19 '12 at 18:25
  • Also compare with Java inner classes. I don't think they break OO design principles *too* badly, but they do have precisely this structure: the inner class can refer to the outer class instance that created it. I don't think there's anything inherently horrible about having a data member that's an instance of an inner class, and I think the design translates to C++. Again, the problem is just that in C++, within the ctor/dtor, that pointer/reference can't do all that you might hope it would do. – Steve Jessop Dec 19 '12 at 18:31

4 Answers4

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.

amaprotu
  • 21
  • 3
  • Oh - OK, can be bad. I only use such things to set a private member to be used later, not for parent calls inside the contained class ctor. – Martin James Dec 19 '12 at 18:02
  • It is, indeed, unclear how complete the parent class is, but that's because the code itself is incomplete. There is no apparent connection between the `Parent` constructor here and the use of a `Parent` pointer as the argument to the `Member` constructor. I suspect that with real code the assertion that begins this answer is wrong. – Pete Becker Dec 19 '12 at 19:06
  • It is not bad because it will always crash (though I'm not confident it would always compile) but it is "bad" because assumptions must be made in the Member class about the state of Parent and that is very messy. – amaprotu Dec 19 '12 at 19:23
2

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.

WhozCraig
  • 65,258
  • 11
  • 75
  • 141
1

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.

Andrei Tita
  • 1,226
  • 10
  • 13
0

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."

Jens
  • 8,423
  • 9
  • 58
  • 78