Command Pattern with Undo , returning response in Invoker and Command class or Callback? I have used command pattern with undo support. The terms associated with Command Pattern are as follows

Client &rarr; Invoker &rarr; Command instance execute() &rarr; Receiver 

so the client will need to know whether the operation was successfully so that it can decide whether to undo.

In my example, Receiver talks to the Volt DB using some APIs, so there are predefined response classes with result code and result message. 

It would be bad if I sent those all the way to the client.

Having Invoker and Command to return boolean indicating success or failure feels better. I have seen an example where they use callbacks. Is this something only for asynchronous operations or I can also use it here? Which would be better?

Snippets of the code are below; full code is available on GitHub.

Invoker class

package com.spakai.undoredo;

import java.util.Stack;

public class Invoker {

    private final Stack&lt;Command&gt; undoStack;
    private final Stack&lt;Command&gt; redoStack;

    public Invoker() {
        undoStack = new Stack&lt;&gt;();
        redoStack = new Stack&lt;&gt;();
    }

    public void execute(Command cmd) {
        undoStack.push(cmd);
        redoStack.clear();
        cmd.execute();
    }

    public void undo() {
        if (!undoStack.isEmpty()) {
            Command cmd = undoStack.pop();
            cmd.undo();
            redoStack.push(cmd);
        }
    }

    public void redo() {
        Command cmd = redoStack.pop();
        cmd.execute();
        undoStack.push(cmd);

    }
}


Receiver interface

package com.spakai.undoredo;

public interface Receiver {
    public CreateGroupResponse createGroup(int groupId, int subscriptionId);
    public DeleteGroupResponse deleteGroup(int groupId);        
    //many more
}


Command interface

package com.spakai.undoredo;

public interface Command {
    public void execute();
    public void undo();    
    public void redo();
}


Sample Command instance

package com.spakai.undoredo;

public class CreateGroupAndSubscription implements Command {

    // which states do i need to store in order to execute and undo
    private int groupId;
    private int subscriptionId;

    // this is the Volt handle apis that talks to VoltDB
    private Receiver receiver;

    public CreateGroupAndSubscription(int groupId, int subscriptionId, Receiver receiver) {
        this.groupId = groupId;
        this.subscriptionId = subscriptionId;
        this.receiver = receiver;
    }

    @Override
    public void execute() {
        CreateGroupResponse response = receiver.createGroup(groupId,subscriptionId);
    }

    @Override
    public void undo() {
        DeleteGroupResponse response = receiver.deleteGroup(groupId);
    }

    @Override
    public void redo() {
        execute();     
    }
}


Any other suggestions on the whole code are most welcomed.
Excellent "by the book" realization of the command pattern with undo/redo ;)

Just 2 comments:


If receiver changes the state of the database, I would definitively handle the error case - the current implementation seems to ignore it. Imagine that undo fails (group was not deleted) and the previous command created the subscription. Then another undo (which tries to delete the subscription) also fails because there is still a group referencing the subscription.
Maybe it is a matter if taste, but I don't see a case where the implementation of execute and redo differs. Usually, I use only 2 methods do and undo where do will be used for execute as well as redo.





  Having Invoker and Command to return boolean indicating success or
  failure feels better. I have seen an example where they use callbacks.
  Is this something only for asynchronous operations or I can also use
  it here? Which would be better?


If a boolean (without error message) is enough, I would prefer the boolean return value because of its simplicity. Otherwise, I would even prefer something like an ErrorInfo object that contains a success flag and an optional error message (or even other information like the exception) as return value.

Another option is to throw an exception - may be also valid in your case. For that kind of error handling, the command should be executed before being pushed to the undoStack obviously.