Diamond inheritance (C++)

Question

I know that having diamond inheritance is considered bad practice. However, I have 2 cases in which I feel that diamond inheritance could fit very nicely. I want to ask, would you recommend me to use diamond inheritance in these cases, or is there another design that could be better.

Case 1: I want to create classes that represent different kinds of "Actions" in my system. The actions are classified by several parameters:

• The action can be "Read" or "Write".
• The action can be with delay or without delay (It is not just 1 parameter. It changes the behavior significantly).
• The action's "flow type" can be FlowA or FlowB.

I intend to have the following design:

// abstract classes
class Action  
{
    // methods relevant for all actions
};
class ActionRead      : public virtual Action  
{
    // methods related to reading
};
class ActionWrite     : public virtual Action  
{
    // methods related to writing
};
class ActionWithDelay : public virtual Action  
{
    // methods related to delay definition and handling
};
class ActionNoDelay   : public virtual Action  {/*...*/};
class ActionFlowA     : public virtual Action  {/*...*/};
class ActionFlowB     : public virtual Action  {/*...*/};

// concrete classes
class ActionFlowAReadWithDelay  : public ActionFlowA, public ActionRead, public ActionWithDelay  
{
    // implementation of the full flow of a read command with delay that does Flow A.
};
class ActionFlowBReadWithDelay  : public ActionFlowB, public ActionRead, public ActionWithDelay  {/*...*/};
//...

Of course, I will obey that no 2 actions (inheriting from Action class) will implement the same method.

Case 2: I implement the composite design pattern for a "Command" in my system. A command can be read, written, deleted, etc. I also want to have a sequence of commands, which can also be read, written, deleted, etc. A sequence of commands can contain other sequences of commands.

So I have the following design:

class CommandAbstraction
{
    CommandAbstraction(){};
    ~CommandAbstraction()=0;
    void Read()=0;
    void Write()=0;
    void Restore()=0;
    bool IsWritten() {/*implemented*/};
    // and other implemented functions
};

class OneCommand : public virtual CommandAbstraction
{
    // implement Read, Write, Restore
};

class CompositeCommand : public virtual CommandAbstraction
{
    // implement Read, Write, Restore
};

In addition, I have a special kind of commands, "Modern" commands. Both one command and composite command can be modern. Being "Modern" adds a certain list of properties to one command and composite command (mostly same properties for both of them). I want to be able to hold a pointer to CommandAbstraction, and initialize it (via new) according to the needed type of command. So I want to do the following design (in addition to the above) :

class ModernCommand : public virtual CommandAbstraction
{
    ~ModernCommand()=0;
    void SetModernPropertyA(){/*...*/}
    void ExecModernSomething(){/*...*/}
    void ModernSomethingElse()=0;

};
class OneModernCommand : public OneCommand, public ModernCommand
{
    void ModernSomethingElse() {/*...*/};
    // ... few methods specific for OneModernCommand
};
class CompositeModernCommand : public CompositeCommand, public ModernCommand
{
    void ModernSomethingElse() {/*...*/};
    // ... few methods specific for CompositeModernCommand
};

Again, I will make sure that no 2 classes inheriting from CommandAbstraction class will implement the same method.

Thank you.

Answer 1

Inheritance is the second strongest (more coupling) relations in C++, preceded only by friendship. If you can redesign into using only composition your code will be more loosely coupled. If you cannot, then you should consider whether all your classes should really inherit from the base. Is it due to implementation or just an interface? Will you want to use any element of the hierarchy as a base element? Or are just leaves in your hierarchy that are real Action's? If only leaves are actions and you are adding behavior you can consider Policy based design for this type of composition of behaviors.

The idea is that different (orthogonal) behaviors can be defined in small class sets and then bundled together to provide the real complete behavior. In the example I will consider just one policy that defines whether the action is to be executed now or in the future, and the command to execute.

I provide an abstract class so that different instantiations of the template can be stored (through pointers) in a container or passed to functions as arguments and get called polymorphically.

class ActionDelayPolicy_NoWait;

class ActionBase // Only needed if you want to use polymorphically different actions
{
public:
    virtual ~Action() {}
    virtual void run() = 0;
};

template < typename Command, typename DelayPolicy = ActionDelayPolicy_NoWait >
class Action : public DelayPolicy, public Command
{
public:
   virtual run() {
      DelayPolicy::wait(); // inherit wait from DelayPolicy
      Command::execute();  // inherit command to execute
   }
};

// Real executed code can be written once (for each action to execute)
class CommandSalute
{
public:
   void execute() { std::cout << "Hi!" << std::endl; }
};

class CommandSmile
{
public:
   void execute() { std::cout << ":)" << std::endl; }
};

// And waiting behaviors can be defined separatedly:
class ActionDelayPolicy_NoWait
{
public:
   void wait() const {}
};

// Note that as Action inherits from the policy, the public methods (if required)
// will be publicly available at the place of instantiation
class ActionDelayPolicy_WaitSeconds
{
public:
   ActionDelayPolicy_WaitSeconds() : seconds_( 0 ) {}
   void wait() const { sleep( seconds_ ); }
   void wait_period( int seconds ) { seconds_ = seconds; }
   int wait_period() const { return seconds_; }
private:
   int seconds_;
};

// Polimorphically execute the action
void execute_action( Action& action )
{
   action.run();
}

// Now the usage:
int main()
{
   Action< CommandSalute > salute_now;
   execute_action( salute_now );

   Action< CommandSmile, ActionDelayPolicy_WaitSeconds > smile_later;
   smile_later.wait_period( 100 ); // Accessible from the wait policy through inheritance
   execute_action( smile_later );
}

The use of inheritance allows public methods from the policy implementations to be accessible through the template instantiation. This disallows the use of aggregation for combining the policies as no new function members could be pushed into the class interface. In the example, the template depends on the policy having a wait() method, which is common to all waiting policies. Now waiting for a time period needs a fixed period time that is set through the period() public method.

In the example, the NoWait policy is just a particular example of the WaitSeconds policy with the period set to 0. This was intentional to mark that the policy interface does not need to be the same. Another waiting policy implementation could be waiting on a number of milliseconds, clock ticks, or until some external event, by providing a class that registers as a callback for the given event.

If you don't need polymorphism you can take out from the example the base class and the virtual methods altogether. While this may seem overly complex for the current example, you can decide on adding other policies to the mix.

While adding new orthogonal behaviors would imply an exponential growth in the number of classes if plain inheritance is used (with polymorphism), with this approach you can just implement each different part separately and glue it together in the Action template.

For example, you could make your action periodic and add an exit policy that determine when to exit the periodic loop. First options that come to mind are LoopPolicy_NRuns and LoopPolicy_TimeSpan, LoopPolicy_Until. This policy method ( exit() in my case ) is called once for each loop. The first implementation counts the number of times it has been called an exits after a fixed number (fixed by the user, as period was fixed in the example above). The second implementation would periodically run the process for a given time period, while the last one will run this process until a given time (clock).

If you are still following me up to here, I would indeed make some changes. The first one is that instead of using a template parameter Command that implements a method execute() I would use functors and probably a templated constructor that takes the command to execute as parameter. The rationale is that this will make it much more extensible in combination with other libraries as boost::bind or boost::lambda, since in that case commands could be bound at the point of instantiation to any free function, functor, or member method of a class.

Now I have to go, but if you are interested I can try posting a modified version.

Answer 2

There's a design-quality difference between implementation-oriented diamond inheritance where implementation is inherited (risky), and subtyping-oriented inheritance where interfaces or marker-interfaces are inherited (often useful).

Generally, if you can avoid the former, you're better off since somewhere down the line the exact invoked method may cause problems, and the importance of virtual bases, states, etc., starts mattering. In fact, Java wouldn't allow you to pull something like that, it supports only the interface hierarchy.

I think that the "cleanest" design you can come up for this is to effectively turn all your classes in the diamond into mock-interfaces (by having no state information, and having pure virtual methods). This reduces the impact of ambiguity. And of course, you can use multiple and even diamond inheritance for this just like you would use implements in Java.

Then, have a set of concrete implementations of these interfaces that can be implemented in different ways (E.g., aggregation, even inheritance).

Encapsulate this framework so that external clients only get the interfaces and never interact directly with the concrete types, and make sure to thoroughly test your implementations.

Of course, this is a lot of work, but if you're writing a central and reusable API, this might be your best bet.

Answer 3

I ran into this problem just this week and found an article on DDJ that explained the problems and when you should or shouldn't be concerned about them. Here it is:

"Multiple Inheritance Considered Useful"

Answer 4

"Diamonds" in the inheritance hierarchy of interfaces is quite safe - it's inheritance of code that get's you into hot water.

To get code reuse, I advise you to consider mixins (google for C++ Mixins if you are unfamiliar with the tequnique). When using mixins you feel like you can "go shopping" for the code snippets that you need to implement you class without using multiple inheritance of stateful classes.

So, the pattern is - multiple inheritance of interfaces and a single chain of mixins (giving you code reuse) to help implement the concrete class.

Hope that helps!

Answer 5

With the first example.....

its whether ActionRead ActionWrite need to be subclasses of action at all.

since you are going to end up with one concrete class that will be an action anyways, you could just inherit actionread and actionwrite without them being actions in themselves.

though, you could invent code that would require them to be actions. But in general I'd try and separate Action, Read, Write, and Delay and just the concrete class mixes all that together

Answer 6

With out knowing more of what you are doing, I would probably reorganize things a bit. Instead of multiple inheritence with all these versions of action, I would make polymorphic reading and writing and writing classes, instanciated as delegates.

Something like the following (which has no diamond inheritence):

Here I present one of many ways implementing optional Delay, and assume the delay methodology is the same for all readers. each subclass might have their own implementation of delay in which case you would pass down to Read and instance of the respective derived Delay class.

class Action // abstract
{
   // Reader and writer would be abstract classes (if not interfaces)
   // from which you would derive to implement the specific
   // read and write protocols.

   class Reader // abstract
   {
      Class Delay {...};
      Delay *optional_delay; // NULL when no delay
      Reader (bool with_delay)
      : optional_delay(with_delay ? new Delay() : NULL)
      {};
      ....
   };

   class Writer {... }; // abstract

   Reader  *reader; // may be NULL if not a reader
   Writer  *writer; // may be NULL if not a writer

   Action (Reader *_reader, Writer *_writer)
   : reader(_reader)
   , writer(_writer)
   {};

   void read()
   { if (reader) reader->read(); }
   void write()
   { if (writer)  writer->write(); }
};


Class Flow : public Action
{
   // Here you would likely have enhanced version
   // of read and write specific that implements Flow behaviour
   // That would be comment to FlowA and FlowB
   class Reader : public Action::Reader {...}
   class Writer : public Action::Writer {...}
   // for Reader and W
   Flow (Reader *_reader, Writer *_writer)
   : Action(_reader,_writer)
   , writer(_writer)
   {};
};

class FlowA :public Flow  // concrete
{
    class Reader : public Flow::Reader {...} // concrete
    // The full implementation for reading A flows
    // Apparently flow A has no write ability
    FlowA(bool with_delay)
    : Flow (new FlowA::Reader(with_delay),NULL) // NULL indicates is not a writer
    {};
};

class FlowB : public Flow // concrete
{
    class Reader : public Flow::Reader {...} // concrete
    // The full implementation for reading B flows
    // Apparently flow B has no write ability
    FlowB(bool with_delay)
    : Flow (new FlowB::Reader(with_delay),NULL) // NULL indicates is not a writer
    {};
};

Answer 7

For case 2, isn't a OneCommand just a special case of CompositeCommand? If you eliminate OneCommand and allow CompositeCommands to only have one element, I think your design gets simpler:

              CommandAbstraction
                 /          \
                /            \
               /              \
        ModernCommand      CompositeCommand
               \               /
                \             /
                 \           /
             ModernCompositeCommand

You still have the dreaded diamond, but I think this may be an acceptable case for it.