[W6.2b][T09-4] Vincent Neo #122
Conversation
… in every class inheriting from Command
|
Hi @tenvinc, your pull request title is invalid. For PR sent to satisfy a learning outcome, the PR name should be in the format of For team PR, the PR name should be in the format of Please follow the above format strictly and edit your title for reprocessing. Note: this comment is posted by a bot. If you believe this is done in error, please create an issue at nus-se-pr-bot and add a link to this PR. |
| @@ -70,4 +70,6 @@ public int getTargetIndex() { | |||
| public void setTargetIndex(int targetIndex) { | |||
| this.targetIndex = targetIndex; | |||
| } | |||
|
|
|||
| public abstract boolean isMutating(); | |||
There was a problem hiding this comment.
Better you add a header comment to both this method and overriding methods since this method returns a very sensitive information and method name does not give the complete picture of the responsibility.
| @@ -85,7 +85,8 @@ public CommandResult execute(String userCommandText) throws Exception { | |||
| private CommandResult execute(Command command) throws Exception { | |||
| command.setData(addressBook, lastShownList); | |||
| CommandResult result = command.execute(); | |||
| storage.save(addressBook); | |||
| if (command.isMutating()) | |||
| storage.save(addressBook); | |||
There was a problem hiding this comment.
Though the curly braces are not required, still it is encouraged to use to increase the readability of the code
| @@ -85,7 +85,8 @@ public CommandResult execute(String userCommandText) throws Exception { | |||
| private CommandResult execute(Command command) throws Exception { | |||
There was a problem hiding this comment.
You need to update the header comment of this method as the behavior of execute changes due to the way data is saved
LO for polymorphism