🚀 go-pugleaf

Mike K wrote:
> This is a student's, not a school question. I'm just learning C++.
> Just a quick example. Remember hearing something about this being wrong, but
> can't rembember. If so, why?
> 
> class MyClass {
>   int x, y, z;
> public:
>   MyClass(void) {
>     Reset();
>   }
> 
>   MyClass(int x, int y) {
>     Reset();
>     _x = x; _y = y;
>   }
> 
>   void Reset(void) {
>     _x = _y = _z = 0;
>   }
> };
> 
> I can't remember what the problem with calling a member function during
> construction is (if any). It seems this way would be easier to add another
> variable to a class, without rewriting all the constructors. Please advise.
> ...

Aside from incorrect member variable names, this particular example doesn't
exhibit any problems.

However, there is a generic design error that is usually illustrated by an
example that uses similar calling pattern. Moreover, I'd say that this
design error is exhibited by your code.

Method 'Reset' is a public method  of the class. This means that this method
can be called form the client code at any time. This, in turn, means that
method 'Reset' can (and will) always assume that the current state of the
'*this' object is a valid state. This should be taken into account when this
method is called form a constructor. Before the method is called it is
important to make sure that the construction steps that have already been
completed have brought the '*this' object to a valid state.

Let's take a look at your code. You call method 'Reset' from class's
constructor. Is the object's state valid at the moment of the call? I'd say
it isn't. The object's data fields contain indeterminate values. In this
particular case you are lucky, because the implementation of method 'Reset'
doesn't rely on the current object state in any way. But what if tomorrow
you (or someone else) decide to change the implementation of this method so
that it starts with dumping the value of 'x * y / z' into a log file and
only then sets all values to, say, 1?

  void Reset()
  {
    assert(z != 0);
    dump(x * y / z);
    x = y = z = 1;
  }

In this case you'll definitely run into problems with the calls that
originate from constructors.

In order to get rid of the problem you should either make sure that the
object is in valid state before the method is called (by explicitly
initializing x, y and z _before_ the call) or break the method into two
separate methods - one is public (assumes that the object is in valid state)
and another is private (makes no such assumption). The constructors should
call the private version, not the public one

  private:

    void Init() // makes no assumptions about the current object state
      { x = y = z = 1; }

  public:

    MyClass()
     { Init(); }

    void Reset() // assumes that the object is in valid state
    {
      assert(z != 0);
      dump(x * y / z);
      Init();
    }

Once again, from the formal point of view, the code from your original post
is fine. Nevertheless, it is a good practice to _always_ assume that public
methods of the class _require_ that the instance of the class is in valid
state at the moment of the call.

-- 
Best regards,
Andrey Tarasevich
Brainbench C and C++ Programming MVP