[W6.1d][T15-B2] Cheng Jin Ting #917
Conversation
|
|
||
|
|
||
| //Making the method abstract forces all child classes to declare whether it is a mutating command or not, rather than depend on an arbitrary decision made in the parent class. | ||
| public abstract boolean isMutating () ; | ||
| } |
There was a problem hiding this comment.
A more precise phrasing is Returns true if … (instead of Checks if …)
It is better to mention it returns true if the Command potentially mutates the data (header comments specify a contract, so try to be as precise as possible). E.g. Adding a duplicate person will not mutate data but the corresponding AddCommand object will still return true for this method.
@return … line can be omitted for this method as it says the same thing as the first sentence.
| @@ -85,7 +85,9 @@ 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()) { | |||
There was a problem hiding this comment.
Coding standard violations! Please refer the coding standard.
| if (new Parser().parseCommand(inputCommand).isMutating()) { | ||
| assertEquals(addressBook, saveFile.load()); | ||
| } | ||
|
|
There was a problem hiding this comment.
This change doesn’t help much because it does not confirm that the saving happens only for mutating commands. Tests will pass even if you don’t have this change. To be complete, you should have an else branch that confirms the file was not updated for non mutating commands (e.g. by checking the last updated time of the file).
|
Some comments added. Please close the PR after reading comments. |
Created isMutating method and updated Junit test