Refactor undo #130
Refactor undo #130
Conversation
|
|
||
|
|
||
| @Override | ||
| public CommandResult execute(ItemModel model) { | ||
| requireNonNull(model); | ||
| beforeClear = model.getItemStorage().deepCopy(); | ||
| System.out.println(beforeClear.size()); |
There was a problem hiding this comment.
Is this line for debugging purposes? Perhaps we should remove it?
|
|
||
| @Override | ||
| public void reverse(ItemModel model) throws CommandException { | ||
| System.out.println(beforeClear.size()); |
| @@ -16,4 +17,7 @@ public CommandResult execute(ItemModel model) { | |||
| return new CommandResult(MESSAGE_EXIT_ACKNOWLEDGEMENT, false, true); | |||
| } | |||
|
|
|||
| @Override | |||
| public void reverse(ItemModel model) throws CommandException { } | |||
|
|
|||
There was a problem hiding this comment.
Perhaps include a comment here to explain why this is an empty code block.
There was a problem hiding this comment.
Actually, I think that this might not be a good practice cause we should not have a method block just for the sake of having one. Perhaps you can consider creating an abstract class called UndoableCommand from which all the command that have reverse extends from while for the rest you can just extend from command. This will also make it easier to catch for error if a command cannot be undo.
| model.findItem(searchString); | ||
| return new CommandResult( | ||
| String.format(Messages.MESSAGE_ITEM_LISTED_OVERVIEW, model.getVisualList().size())); | ||
| } | ||
|
|
||
| @Override | ||
| public void reverse(ItemModel model) throws CommandException { | ||
| model.setVisualizeList(beforeFilter); |
| } | ||
|
|
||
| @Override | ||
| public CommandResult execute(ItemModel model) throws CommandException { | ||
| if (elisaStateHistory.size() <= 1) { | ||
| if (elisaCommandHistory.size() <= 0) { |
There was a problem hiding this comment.
I think in this case it is fine to use == instead as there is no way that the size will be less than 0?
| return new CommandResult("Undo successful!", true); | ||
| Command lastDone = elisaCommandHistory.popCommand(); | ||
| lastDone.reverse(model); | ||
| return new CommandResult("Undo successful!"); |
There was a problem hiding this comment.
Now that you are able to undo the command, maybe you can consider adding the command that you just undo so that the user is able to tell what was the command that was undone. I believe that it can be done by Command.COMMAND_WORD to get the command.
There was a problem hiding this comment.
Command class doesn't have the COMMAND_WORD, I will create a getter for it
| * */ | ||
| @Override | ||
| public void pushCommand(Command command) { | ||
| if (!(command instanceof UndoCommand || command instanceof ExitCommand)) { |
There was a problem hiding this comment.
This is one case where having a separate abstract class called UndoableCommand might be good.
| if (undoStack.size() > 0) { | ||
| return undoStack.pop(); | ||
| } else { | ||
| return null; |
There was a problem hiding this comment.
Perhaps consider throwing the exception that there is no command in the undo stack from here so that there is no need to do 2 checks within the code.
There was a problem hiding this comment.
actually I don't need to check here since the only usage of pop is inside UndoCommand where there is the check for empty stack
| @@ -17,6 +17,6 @@ | |||
| */ | |||
| public abstract CommandResult execute(ItemModel model) throws CommandException; | |||
|
|
|||
| public abstract void reverse(ItemModel model) throws CommandException; | |||
| public abstract String getCommandWord(); | |||
There was a problem hiding this comment.
I think this should have been implemented in UndoableCommand but just change it in the next iteration.
| lastDone.reverse(model); | ||
| return new CommandResult("Undo successful!"); | ||
| return new CommandResult("Undo [" + lastDone.getCommandWord() + "] command successful!"); |
There was a problem hiding this comment.
Can attempt using String.format() next time to make it less messy.
Fixes #133