C++ : Call a base class method automatically ?

Question

Here we go,

I'm trying to implement the command design pattern, but I'm stumbling accross a conceptual problem.

Let's say you have a base class and a few subclasses like in the example bellow :

class Command : public boost::noncopyable {
    virtual ResultType operator()()=0;

    //restores the model state as it was before command's execution
    virtual void undo()=0;

    //registers this command on the command stack
    void register();
};


class SomeCommand : public Command {
    virtual ResultType operator()(); // implementation doesn't really matter here
    virtual void undo(); // same 
};

The thing is, everytime operator () is called on a SomeCommand instance, I'd like to add *this to a stack (mostly for undo purposes) by calling the Command's register method.

I'd like to avoid calling "register" from SomeCommand::operator()() but to have it called automaticaly (someway ;-) )

I know that when you construct a sub class such as SomeCommand, the base class constructor is called automaticaly, so I could add a call to "register" there. The thing I don't want to call register until operator()() is called.

Do you have an idea on how to do this ? I guess my design is somewhat flawed but I don't really know how to make this work

Answer 1

It looks as if you can benefit from the NVI (Non-Virtual Interface) idiom. There the interface of the command object would have no virtual methods, but would call into private extension points:

class command {
public:
   void operator()() {
      do_command();
      add_to_undo_stack(this);
   }
   void undo();
private:
   virtual void do_command();
   virtual void do_undo();
};

There are different advantages to this approach, first of which is that you can add common functionality in the base class. Other advantages are that the interface of your class and the interface of the extension points is not bound to each other, so you could offer different signatures in your public interface and the virtual extension interface. Search for NVI and you will get much more and better explanations.

Addendum: The original article by Herb Sutter where he introduces the concept (yet unnamed)

Answer 2

Split the operator in two different methods, e.g. execute and executeImpl (to be honest, I don't really like the () operator). Make Command::execute non-virtual, and Command::executeImpl pure virtual, then let Command::execute perform the registration, then call it executeImpl, like this:

class Command
   {
   public:
      ResultType execute()
         {
         ... // do registration
         return executeImpl();
         }
   protected:
      virtual ResultType executeImpl() = 0;
   };

class SomeCommand
   {
   protected:
      virtual ResultType executeImpl();
   };

Answer 3

Assuming it's a 'normal' application with undo and redo, I wouldn't try and mix managing the stack with the actions performed by the elements on the stack. It will get very complicated if you either have multiple undo chains (e.g. more than one tab open), or when you do-undo-redo, where the command has to know whether to add itself to undo or move itself from redo to undo, or move itself from undo to redo. It also means you need to mock the undo/redo stack to test the commands.

If you do want to mix them, then you will have three template methods, each taking the two stacks (or the command object needs to have references to the stacks it operates on when created), and each performing the move or add, then calling the function. But if you do have those three methods, you will see that they don't actually do anything other than call public functions on the command and are not used by any other part of the command, so become candidates the next time you refactor your code for cohesion.

Instead, I'd create an UndoRedoStack class which has an execute_command(Command*command) function, and leave the command as simple as possible.

Answer 4

Basically Patrick's suggestion is the same as David's which is also the same as mine. Use NVI (non-virtual interface idiom) for this purpose. Pure virtual interfaces lack any kind of centralized control. You could alternatively create a separate abstract base class that all commands inherit, but why bother?

For detailed discussion about why NVIs are desirable, see C++ Coding Standards by Herb Sutter. There he goes so far as to suggest making all public functions non-virtual to achieve a strict separation of overridable code from public interface code (which should not be overridable so that you can always have some centralized control and add instrumentation, pre/post-condition checking, and whatever else you need).

class Command 
{
public:
   void operator()() 
   {
      do_command();
      add_to_undo_stack(this);
   }

   void undo()
   {
      // This might seem pointless now to just call do_undo but 
      // it could become beneficial later if you want to do some
      // error-checking, for instance, without having to do it
      // in every single command subclass's undo implementation.
      do_undo();
   }

private:
   virtual void do_command() = 0;
   virtual void do_undo() = 0;
};

If we take a step back and look at the general problem instead of the immediate question being asked, I think Pete offers some very good advice. Making Command responsible for adding itself to the undo stack is not particularly flexible. It can be independent of the container in which it resides. Those higher-level responsibilities should probably be a part of the actual container which you can also make responsible for executing and undoing the command.

Nevertheless, it should be very helpful to study NVI. I've seen too many developers write pure virtual interfaces like this out of the historical benefits they had only to add the same code to every subclass that defines it when it need only be implemented in one central place. It is a very handy tool to add to your programming toolbox.

Answer 5

I once had a project to create a 3D modelling application and for that I used to have the same requirement. As far as I understood when working on it was that no matter what and operation should always know what it did and therefore should know how to undo it. So I had a base class created for each operation and it's operation state as shown below.

class OperationState
{
protected:
    Operation& mParent;
    OperationState(Operation& parent);
public:
    virtual ~OperationState();
    Operation& getParent();
};

class Operation
{
private:
    const std::string mName;
public:
    Operation(const std::string& name);
    virtual ~Operation();

    const std::string& getName() const{return mName;}

    virtual OperationState* operator ()() = 0;

    virtual bool undo(OperationState* state) = 0;
    virtual bool redo(OperationState* state) = 0;
};

Creating a function and it's state would be like:

class MoveState : public OperationState
{
public:
    struct ObjectPos
    {
        Object* object;
        Vector3 prevPosition;
    };
    MoveState(MoveOperation& parent):OperationState(parent){}
    typedef std::list<ObjectPos> PrevPositions;
    PrevPositions prevPositions;
};

class MoveOperation : public Operation
{
public:
    MoveOperation():Operation("Move"){}
    ~MoveOperation();

    // Implement the function and return the previous
    // previous states of the objects this function
    // changed.
    virtual OperationState* operator ()();

    // Implement the undo function
    virtual bool undo(OperationState* state);
    // Implement the redo function
    virtual bool redo(OperationState* state);
};

There used to be a class called OperationManager. This registered different functions and created instances of them within it like:

OperationManager& opMgr = OperationManager::GetInstance();
opMgr.register<MoveOperation>();

The register function was like:

template <typename T>
void OperationManager::register()
{
    T* op = new T();
    const std::string& op_name = op->getName();
    if(mOperations.count(op_name))
    {
        delete op;
    }else{
        mOperations[op_name] = op;
    }
}

Whenever a function was to be executed, it would be based on the currently selected objects or the whatever it needs to work on. NOTE: In my case, I didn't need to send the details of how much each object should move because that was being calculated by MoveOperation from the input device once it was set as the active function.
In the OperationManager, executing a function would be like:

void OperationManager::execute(const std::string& operation_name)
{
    if(mOperations.count(operation_name))
    {
        Operation& op = *mOperations[operation_name];
        OperationState* opState = op();
        if(opState)
        {
            mUndoStack.push(opState);
        }
    }
}

When there's a necessity to undo, you do that from the OperationManager like:
OperationManager::GetInstance().undo();
And the undo function of the OperationManager looks like this:

void OperationManager::undo()
{
    if(!mUndoStack.empty())
    {
        OperationState* state = mUndoStack.pop();
        if(state->getParent().undo(state))
        {
            mRedoStack.push(state);
        }else{
            // Throw an exception or warn the user.
        }
    }
}

This made the OperationManager not be aware of what arguments each function needs and so was easy to manage different functions.