Is it good practice to not initialize every member in a C++ constructor?

Question

I have a class that has two states, and different members are only applicable in one state or the other.

Which is the better practice:

• Option 1: constructor initializes only the members relevant to the first (initial) state
• Option 2: initialize every member, even if that means inventing "uninitialized" values for members?

E.g.

class Foo {
public:
  enum State { A, B };

  // Constructor
  // Option 1: Only initialize first state
  Foo(int a1) : _state(A), _a1(a1) {}
  // ... or ... 
  // Option 2: initialize every member
  Foo(int a1) : _state(A), _a1(a1), b1(-1), b2(-1) {}

  State getState() const { return _state; }

  // Only applicable if State is A
  int getA1() const { return _a1; } // Might also add assert that State is A

  // Only applicable if State is B
  int getB1() const { return _b1; } // Might also add assert that State is B
  int getB2() const { return _b2; } // Might also add assert that State is B

private:
  State _state;

  int _a1;

  int _b1;
  int _b2;
};

Answer 1

Reading from an uninitialized variable is undefined behavior, so if you use option 1 and then someone calls getB1() or getB2(), you have undefined behavior.

There is nothing inherently wrong with option 1 as long as you clearly document that calling these getters might invoke undefined behavior and in what circumstances that can happen. This way you are moving the burden of ensuring defined behavior to the consumer of this class.

You could also store flags indicating if they've been initialized and throw an exception if a read is attempted before they are initialized. This way you would get a clearly defined error instead of UB. (You could also use boost::optional<int> here instead, which takes care of providing this extra flag for you.)

Considering all of these points, going with a "dummy" value might be preferred simply because there is no risk of undefined behavior and results in a simpler implementation. (If you do go with a dummy value, make sure you provide a static constant value so that callers can compare to see if the value hasn't been set.)

Answer 2

This might be a case where using a union as a member is applicable. You can use a union structure to save memory space while having an assertion check the state of which union members to use.

struct BMembers {
  int _b1;
  int _b2;

};

union MyUnion {
  int _a1;
  BMembers members;
};

class Foo {
public:
  enum State { A, B };

  Foo(int a1) : _state(A), myUnion._a1(a1) {}

  State getState() const { return _state; }

  // Only applicable if State is A
  int getA1() const { return myUnion._a1; } // Might also add assert that State is A

  // Only applicable if State is B
  int getB1() const { return myUnion._b1; } // Might also add assert that State is B
  int getB2() const { return myUnion._b2; } // Might also add assert that State is B

private:
  State _state;

  MyUnion myUnion;
};

Answer 3

It's a good practice to initialize everything. Maybe your object has no real value that would count as "uninitialized" and that would break your code. As a solution, consider using separate classes to define each state. That way you document better what is required for each state, and, depending on the size of your members, can save space by storing only what you need:

class Foo{
public:
  enum State{A,B};
  virtual State getState() const = 0;
  virtual int getA1() const = 0;
  virtual int getB1() const = 0;
  virtual int getB2() const = 0;
};

class Foo_A : public Foo{
public:
  Foo_A(int a1) : _a1(a1) {} // State implicit
  State getState() const {return A;}
  int getA1() const {return _a1;}
  int getB1() const {throw "Bad Call";} // For simplicity. You should use a class derived from std::exception;
  int getB2() const {throw "Bad Call";}
private:
  int _a1;
};

class Foo_B : public Foo{
public:
  Foo_B(int b1, int b2) : _b1(b1), _b2(b2) {}
  State getState() const {return B;}
  int getA1() const {throw "Bad Call";}
  int getB1() const {return _b1;}
  int getB2() const {return _b2;}
private:
  int _b1;
  int _b2;
};

Answer 4

I would way it is good practice to initalise everything.

For a state give those values an invalid one that should not be set

PS: For initialisation list do them in the same order as their declaration.

Answer 5

It depends on your particular situation. Initializing each and every variable at its declaration is a good practise. In a big program, we may forget to initialize variables, that may result in code dumps. Later we will want to take extra effort to figure out these initialization issues. So initializing a variable at its declaration itself is a good coding practise. For this we use constructor for classes.

In this particular case, the 2nd option is better. What happens otherwise if you call getB1() or getB2(), they return garbage values.

If that is damn sure that it wont get called, its okay with that. But its better initialize them.

Answer 6

Leaving any member uninitialised is a prime opportunity for any function with sufficient access (members, friends, etc) to attempt to access that member before it is initialised. That causes undefined behaviour - usually in code outside the constructor. Since the root cause of the problem (uninitialised in constructor) and the trigger (access) are in different locations in code, that means bugs that are difficult to identify, therefore hard to correct.

Generally, therefore, it is better practice to initialise all members in a constructor.

However, that need not mean "inventing" values for some members. The better practice is to avoid creating an object until all information needed to construct it is available.