Solution

This Item illustrates a too-common pitfall in OO class relationship design. To recap, classes Protocol1 and Protocol2 are publicly derived from a common base class BasicProtocol, which performs some common work.

A key to identifying the problem is the following:

Each DoMsg…() member function calls the BasicProtocol::Basic…() functions as needed to perform the common work, but the DoMsg…() function performs the actual transmissions itself.

Here we have it: This is clearly describing an "is implemented in terms of " relationship, which, in C++, is spelled either "private inheritance" or "membership." Unfortunately, many people still frequently misspell it as "public inheritance," thereby confusing implementation inheritance with interface inheritance. The two are not the same thing, and that confusion is at the root of the problem here.

Common Mistake

Never use public inheritance except to model true Liskov IS-A and WORKS-LIKE-A. All overridden member functions must require no more and promise no less.

Incidentally, programmers in the habit of making this mistake (using public inheritance for implementation) usually end up creating deep inheritance hierarchies. This greatly increases the maintenance burden by adding unnecessary complexity, forcing users to learn the interfaces of many classes even when all they want to do is use a specific derived class. It can also have an impact on memory use and program performance by adding unnecessary vtables and indirections to classes that do not really need them. If you find yourself frequently creating deep inheritance hierarchies, you should review your design style to see if you've picked up this bad habit. Deep hierarchies are rarely needed and almost never good. And if you don't believe that but think that "OO just isn'tOO without lots of inheritance," then a good counter-example to consider is the standard library itself.

Guideline

Never inherit publicly to reuse code (in the base class); inherit publicly in order to be reused (by code that uses base objects polymorphically).[4] [4] I got this guideline from (and use it with thanks to) Marshall Cline, who is co-author of the classic C++ FAQs book (Cline 95).

In a little more detail, here are several clues that help indicate this problem.

1. BasicProtocol provides no virtual functions (other than the destructor, which we'll get to in a minute).^[5] This means that it is not intended to be used polymorphically, which is a strong hint against public inheritance.

   ^[5] Even if BasicProtocol were itself derived from another class, we come to the same conclusion, because it still does not provide any new virtual functions. If some base class does provide virtual functions, then it's that remote base class that's intended to be used polymorphically, not BasicProtocol itself. So, if anything, we should be deriving from that remote base instead.

2. BasicProtocol has no protected functions or members. This means that there is no "derivation interface," which is a strong hint against any inheritance at all, either public or private.

3. BasicProtocol encapsulates common work, but as described, it does not seem to actually perform its own transmissions as the derived classes do. This means that a BasicProtocol object does not WORK-LIKE-A derived protocol object, nor is it USABLE-AS-A derived protocol object. Public inheritance should be used to model one thing and one thing only—a true interface IS-A relationship that obeys the Liskov Substitution Principle. For greater clarity, I usually call this WORKS-LIKE-A or USABLE-AS-A.^[6]

   ^[6] Yes, sometimes, when you inherit publicly to get an interface, some implementation can come along, too, if the base class has both an interface that you want and some implementation of its own. This is nearly always possible to design away (see the next Item), but it's not always necessary to take the true purist approach of "one responsibility per class."

4. The derived classes use BasicProtocol's public interface only. This means that they do not benefit from being derived classes, and could as easily perform their work using a separate helper object of type BasicProtocol.

This means we have a few clean-up issues. First, because BasicProtocol is clearly not designed to be derived from, its virtual destructor is unnecessary (indeed, misleading) and should be eliminated. Second, BasicProtocol should probably be renamed to something less misleading, such as MessageCreator or MessageHelper.

Once we've made those changes, which option should be used to model this "is implemented in terms of " relationship—private inheritance or membership? The answer is pretty easy to remember.

Guideline

When modeling "is implemented in terms of," always prefer membership/containment, not inheritance. Use private inheritance only when inheritance is absolutely necessary—that is, when you need access to protected members or you need to override a virtual function. Never use public inheritance for code reuse.

Using membership forces a better separation of concerns, because the using class is a normal client with access to only the used class's public interface. Prefer it, and you'll find that your code is cleaner, easier to read, and easier to maintain. In short, your code will cost less.