3

In my C++ application I have an interface that looks like this:

class ICalculator
   {
   public:
      virtual double calculateValue(double d) = 0;
   };

I have implementations of this interface that look like this:

class MySpecificCalculator
   {
   public:
      virtual double calculateValue(double d);
   };

Now my colleague complains about this and tells me it's better to have the calculateValue method protected. That way, we can guarantee that the callers always pass via the interface and not via the direct implementation.

Is this a correct observation? Is it really better to make the implementation of an interface protected? Or can't we even make it private then?

Chubsdad
  • 24,777
  • 4
  • 73
  • 129
Patrick
  • 23,217
  • 12
  • 67
  • 130

2 Answers2

10

Your colleague is right.

Never make virtual functions public.

Guideline #1: Prefer to make interfaces nonvirtual, using Template Method.

Guideline #2: Prefer to make virtual functions private.

Guideline #3: Only if derived classes need to invoke the base implementation of a virtual function, make the virtual function protected.

For the special case of the destructor only:

Guideline #4: A base class destructor should be either public and virtual, or protected and nonvirtual.

Chubsdad
  • 24,777
  • 4
  • 73
  • 129
  • +1, beat me to it while I was adding the `virtual` keyword to the OP's question :p – Matthieu M. Nov 10 '10 at 08:41
  • @Matthieu M: Hmm, I anticipated virtuality there...I should be careful – Chubsdad Nov 10 '10 at 08:43
  • I indeed forgot the 'virtual' keyword. Stupid me. Thanks for the link. – Patrick Nov 10 '10 at 08:58
  • Taking the example in the link: wouldn't it be better to put the Process method in a separate 'front-end' class, and keeping the class as a pure virtual interface? Possibly, there might be another implementation of Process which may need the same implementations of DoProcessPhase1 and DoProcessPhase2), and you can't extend this further if you keep the frond-end part and the interface part within one class. – Patrick Nov 10 '10 at 09:17
  • An interface is nothing but a front-end. – Chubsdad Nov 10 '10 at 09:27
  • @Chubsdad: From guideline 2, does overriding works when we use private virtual functions ? – bjskishore123 Nov 10 '10 at 09:41
  • @Chubsdad: true, but not necessarily the front-end you may want to make public to the rest of the application. Possibly it's only the front-end for another class that wants to use the interface. – Patrick Nov 10 '10 at 09:42
  • @bjskishore123 : Yes. overriding works with base class private virtual functions – Chubsdad Nov 10 '10 at 10:08
  • @Patrick: indeed, what you are talking about (non virtual front-end + pure interface) is usually combined with the PIMPL idiom. You use a Factory to instantiate the right version of derived class in your front-end object. This allows to "seal" the class by hiding the derived classes from the user of the front-end. It also means that the front-end is very simple, and will remain binary-compatible even when adding new derived classes, which means that client code can integrate a new version of your library by simply relinking (or even changing the symlink :p) – Matthieu M. Nov 10 '10 at 16:25
0

It sounds that your colleague means:

class ICalculator
{
public:
    virtual double calculateValue(double d) const = 0;
};

class MySpecificCalculator : public ICalculator
{
protected:
    double calculateValue(double d) const;
};

void Calc(double d, const ICalculator& c)
{
    std::cout << "Result = " << c.calculateValue(d) << std::endl;
}

int main ()
{
    MySpecificCalculator c;
    c.calculateValue(2.1);  // dont do this
    Calc(2.1, c);  // do this instead
    return 0;
}

I cant see any benefit from this design. Why not to be able to call calculate from a concrete reference. To move calculateValue to protected in the derived class, breaks the contract of the base class. You can't use a MySpecificCalculator to calculate a Value, but its base class says so.

Btw this behaviour is not modeled by the NVI idiom explained by Chubsdad (which is the 'correct' way).

hansmaad
  • 18,417
  • 9
  • 53
  • 94