45

My gut feeling is it is not. I am in the following situation:

class PluginLoader
{
   public:
      Builder* const p_Builder;
      Logger* const p_Logger;

      //Others
};

PluginLoader::PluginLoader(Builder* const pBuilder)
   :p_Builder(pBuilder), p_Logger(pBuilder->GetLogger())
{
   //Stuff
}

Or should I change the constructor and pass a Logger* const from where PluginLoader is constructed?

nakiya
  • 14,063
  • 21
  • 79
  • 118

3 Answers3

42

That's perfectly fine and normal. p_Builder was initialized before it.

GManNickG
  • 494,350
  • 52
  • 494
  • 543
  • Gee. I must be a `not` gate. :) – nakiya Nov 12 '10 at 06:00
  • 11
    It's even more so, since he's calling `pBuilder->GetLogger()`, not `p_Builder->GetLogger()`. Both are legal, but the second is sensitive to the instance variable being reordered in the class definition. – Eclipse Nov 12 '10 at 06:02
  • @Eclipse: Oh, I didn't even see that ha. @nakiya: Did you mean to use the member or parameter? Either is safe, as is. – GManNickG Nov 12 '10 at 06:05
  • So what is the difference between assigning in constructor body vs assigning in parameter list? Other than having the ability to assign to `const` members? – nakiya Nov 12 '10 at 06:07
  • @GMan: No, I was worried about calling functions in general there. Because I don't know what's the difference. – nakiya Nov 12 '10 at 06:08
  • @nakiya entries in the initialization list are guaranteed to be initialized in that order. – Martin Beckett Nov 12 '10 at 06:35
  • 8
    @Martin: Close; they're initialized in the order they appear in the class definition. The initializer list should match that, but strictly speaking it doesn't have to. – GManNickG Nov 12 '10 at 07:08
38

What you have is fine. However, I just want to warn you to be careful not to do this: (GMan alluded to this, I just wanted to make it perfectly clear)

class PluginLoader
{
   public:
      Logger* const p_Logger;   // p_Logger is listed first before p_Builder
      Builder* const p_Builder;

      //Others
};

PluginLoader::PluginLoader(Builder* const pBuilder)
   :p_Builder(pBuilder),
    p_Logger(p_Builder->GetLogger())   // Though listed 2nd, it is called first.
                                       // This wouldn't be a problem if pBuilder 
                                       // was used instead of p_Builder
{
   //Stuff
}

Note that I made 2 changes to your code. First, in the class definition, I declared p_Logger before p_Builder. Second, I used the member p_Builder to initialize p_Logger, instead of the parameter.

Either one of these changes would be fine, but together they introduce a bug, because p_Logger is initialized first, and you use the uninitialized p_Builder to initialize it.

Just always remember that the members are initialized in the order they appear in the class definition. And the order you put them in your initialization list is irrelevant.

jmstoker
  • 3,315
  • 22
  • 36
Benjamin Lindley
  • 101,917
  • 9
  • 204
  • 274
1

Perfectly good practice.

I would suggest this (but its on a purely personal level):

instead of having functions called in your constructor, to group them in a init function, only for flexibility purposes: if you later have to create other constructors.

Jason Rogers
  • 19,194
  • 27
  • 79
  • 112
  • 4
    If what you are suggesting is a *private* `init` function that you call from the constructor, I don't quite care for that but I guess it is ok. If what you are suggesting is a *public* `init` function that has to be called by users of the class, that is prone to errors and inconsistencies (after creating an object it would be in a yet invalid state and the user can forget to call the `init` or try to use the object between construction and initialization...) – David Rodríguez - dribeas Nov 12 '10 at 09:15
  • you can use a dummy member, and have the init method return a type. For example `class IDGenerator { public: IDGenerator(); bool reset(); std::uint32_t generate(); private: bool mReset; T_ID_set mIds; std::uint32_t mId; }; IDGenerator::IDGenerator() : mReset(reset()) { } bool IDGenerator::reset() { mIds.clear(); mId = 0; return true; }` – dgsomerton May 10 '16 at 22:34
  • Correct me if I am wrong but the possible problem with your solution is that a member will be constructed by default and then constructed once again in your `init` function. – Wodzu Mar 08 '22 at 12:27