5
\$\begingroup\$

Asking for a review of my implementation of a command pattern. In a Editor, words can be entered and the entering of words can be undone.

General purpose Interface

public interface Command {
    void execute();
}

The concrete write word command

// Command
public class WriteCommand implements Command {

    private Editor editor;
    private String word;

    public WriteCommand(String w, Editor e) {
        this.word = w;
        this.editor = e;
    }

    public String getWord() {
        return this.word;
    }

    public void execute() {
        this.editor.addToWordList(this.getWord());
    }

}

The undo command

public class UndoCommand implements Command {

    private Editor editor;


    public UndoCommand(Editor e) {
        this.editor = e;
    }

    @Override
    public void execute() {
        editor.removeFromWordList();
    }
}

The editor (i.e. receiver, i believe):

// Receiver
public class Editor {

    private List<String> wordList;

    public Editor() {
        wordList = new ArrayList<>();
    }

    public void addToWordList(String w) {
        this.wordList.add(w);
    }

    public void removeFromWordList() {
        this.wordList.remove(this.wordList.size()-1);
    }

    List<String> getWordList() {
        return this.wordList;
    }

}

The invoker keeping the command queue:

public class Invoker {
    private List<Command> undoList;
    private List<Command> redoList;


    public Invoker() {
        undoList = new ArrayList<>();
        redoList = new ArrayList<>();

    }

    public void executeCommand(Command c) {
        undoList.add(c);
        c.execute();
    }
}

The client responsible for creating the commands:

public class Client {
    public static void main(String[] args) {

        Editor e = new Editor();

        Invoker invoker = new Invoker();

        invoker.executeCommand(new WriteCommand("Hello", e));
        invoker.executeCommand(new WriteCommand("World", e));
        System.out.println(e.getWordList());

        invoker.executeCommand(new UndoCommand(e));
        System.out.println(e.getWordList());

    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Seems to fit the bill. As general remarks: I would not include a reference to the editor in the Command itself - it makes more sense to make it a parameter for the execute method. That's more flexible, and you would have less trouble e.g. serializing the Command instances that way. Of course, you should not put the UndoCommand instances in the undoList. But to me, this seems to be on the right track. \$\endgroup\$ – Maarten Bodewes Jan 27 at 1:28
  • 1
    \$\begingroup\$ Same as @MaartenBodewes, passing the "context" as parameter is more flexible than having it as an instance field. Just for the undo, it seems strange to use removeFromWordList your undo maye be used to undo other changes but also to remove another word; your user may want to have a menu to undo "Hello".. (think of your browser's history, you can go back to teh last "n" entries) \$\endgroup\$ – gervais.b Jan 27 at 7:45
  • 1
    \$\begingroup\$ execute() should not take any arguments. Thats correct. Some commands may work over editor, some commands may work over other things. If execute accepted the editor than all commands have to. And that is not "more flexible". On other hand the undo logic should be contained within the commands themselves. Check my PoC PHP implementation, it is much more generic, but it should be well explanatory. github.com/slepic/PhpX/blob/master/src/CommandPattern/Undo/… \$\endgroup\$ – slepic Jan 27 at 10:31
  • \$\begingroup\$ The requirement for command pattern is that the command object contains all required state in itself. Adding parameters to the method breaks this requirement and it stops being a command pattern. \$\endgroup\$ – TorbenPutkonen Jan 27 at 12:56
  • \$\begingroup\$ @TorbenPutkonen The editors state is not part of the state of the command itself. There are certainly reasons to choose one over the other, but that's just bad argumentation. Similarly, I would not want every command in the same queue, so mixing commands for various parts of the program is not recommended. I don't want to have the "Save" action be in my Edit -> Undo queue, for one. \$\endgroup\$ – Maarten Bodewes Jan 27 at 14:59
1
\$\begingroup\$

If this was a school exercise

It's a basic command pattern without any safety checks against programming errors. :) It would be a good idea to check that a word can not be removed from an empty list.

If this was a job interview assignment

While the code shows that you know how to code a command pattern it doesn't tell me if you understand why one would want to use it. Having WriteCommand and UndoCommand objects going around in isolation makes very little sense. And domain-wise, writing to a text editor with command objects is probably not something that I would do.

Instead put the command pattern into actual use by, for example, integrating it to the editor in the form of an undo/redo stack. Appending strings to the editor automatically create undo commands into the stack and performing the undo command executes the last command and adds a corresponding redo command into the redo stack.

| improve this answer | |
\$\endgroup\$

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.