[W5][W17-1]Chen Caijie #143
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rbth7e5
changed the title
Wklee96
reviewed
Feb 12, 2019
|
Because of the way I implemented the edit command, it extracts the person out first by removing it then adding a new person with the new name but with all the same details. the add function throws a duplicate person exception so I needed to catch it. I guess this will be caught when u try and edit to the same name! Thank you for your question!
… On Feb 12, 2019, at 10:13 PM, Wei Kang Lee ***@***.***> wrote:
@Wklee96 commented on this pull request.
I saw that you have included a catch for DuplicatePersonException inside EditCommand. Can I clarify under what situation will this exception be caught?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
sharan8
reviewed
Feb 17, 2019
| package seedu.addressbook.commands; | ||
|
|
||
| import seedu.addressbook.data.exception.IllegalValueException; | ||
| import seedu.addressbook.data.person.*; |
There was a problem hiding this comment.
Try to be explicit in your imports, avoiding the use of .*.
sharan8
reviewed
Feb 17, 2019
| import seedu.addressbook.data.exception.IllegalValueException; | ||
| import seedu.addressbook.data.person.*; | ||
|
|
||
| public class EditCommand extends Command { |
There was a problem hiding this comment.
Including a javadoc comment for new classes would be good.
sharan8
reviewed
Feb 17, 2019
| @@ -73,33 +78,36 @@ public Command parseCommand(String userInput) { | |||
|
|
|||
| switch (commandWord) { | |||
|
|
|||
| case AddCommand.COMMAND_WORD: | |||
| return prepareAdd(arguments); | |||
| case AddCommand.COMMAND_WORD: | |||
There was a problem hiding this comment.
Note that the coding style specifies that there shouldn't be indentation for case clauses.
sharan8
approved these changes
Feb 17, 2019
There was a problem hiding this comment.
Hi Caijie, some comments:
- Good that you updated the User Guide, I/O tests and JUnit tests.
- Note the coding style requirements for switch-case statements - there shouldn't be indentations for case clauses. It would be helpful to configure your IDE accordingly.
- Testing with valid input could be a good addition to your JUnit tests.
- It would be helpful to check if you can handle the case where the name and new name inputs are the same, differently (E.g. Give the user a more personalised feedback message).
- Good that you used a branch to implement this enhancement.
- Think about whether you can break down your commits further such that each one corresponds to one incremental change.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
currently only editing of a person's name is supported.