[W6.4d][F11-B3] Tan Ci Kang #545
Conversation
|
Hi @e0052813, your pull request title is invalid. For phase A, it should be in the format of For phase B, it should be in the format of Please follow the instructions given strictly and edit your title for reprocessing. Submit only one learning outcome per pull request (unless otherwise stated in instructions) and do remember to create your branches from the commit where the Note: this comment is posted by a bot. If you believe this is done in error, please create an issue at cs2103-pr-bot and add a link to this PR. |
|
Hi @e0052813, Your Github username is not recognized. Please post here. Note: this comment is posted by a bot. If you believe this is done in error, please create an issue at cs2103-pr-bot and add a link to this PR. |
There was a problem hiding this comment.
Overall good effort. As mentioned, try to think of a more efficient way of doing this. Also, how should you test the immutable commands after you make the change. Generally, you should think about testing both branches, i.e. mutable case and immutable case. You can close the PR after reading the comments.
| * - the internal address book data are same as those in the {@code expectedAddressBook} <br> | ||
| * - the internal 'last shown list' matches the {@code expectedLastList} <br> | ||
| */ | ||
| private void assertCommandBehaviorWithoutSave(String inputCommand, |
There was a problem hiding this comment.
Instead of doing this, you can save a lot of code by using the original assert method but check the mutability of the command inside this method. You can refer your peers' code on this if you are not sure. Good effort anyways!
| @@ -70,4 +70,9 @@ public int getTargetIndex() { | |||
| public void setTargetIndex(int targetIndex) { | |||
| this.targetIndex = targetIndex; | |||
| } | |||
|
|
|||
| /** | |||
| * Returns true if the data is mutated | |||
There was a problem hiding this comment.
You have added the header comments correctly. But it can be more accurate. Think of a potential case where you will add a duplicate person to the unique list. Will this cause any mutation? Therefore, adding a word "potentially" would be better.
No description provided.