2
\$\begingroup\$

I have used command pattern with undo support. The terms associated with Command Pattern are as follows

Client → Invoker → Command instance execute() → 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<Command> undoStack;
    private final Stack<Command> redoStack;

    public Invoker() {
        undoStack = new Stack<>();
        redoStack = new Stack<>();
    }

    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.

\$\endgroup\$

migrated from stackoverflow.com Aug 11 '17 at 5:52

This question came from our site for professional and enthusiast programmers.

2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Yeah. thinking along the same lines , in execute() , i should run cmd.execute() , check if it was successful first before adding to undo and redo stack. \$\endgroup\$ – spakai Aug 11 '17 at 7:26
  • \$\begingroup\$ Yes, but probably you want also to show the user that the command didn't succeeded (probably with a reason). Than it is not enough to handle it within the class Invoker. The error must be propagated to and handled by the calling code. \$\endgroup\$ – JanDotNet Aug 11 '17 at 7:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.