Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[W5][T11-2]Li Guanlong #34

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

[W5][T11-2]Li Guanlong #34

wants to merge 4 commits into from

Conversation

liguanlong
Copy link

Added a new command: update, which is used to update an existing contact at the specified index.

@liguanlong liguanlong changed the title [W5][T11-3]Li Guanlong [W5][T11-2]Li Guanlong Feb 10, 2019
Copy link

@leerachel leerachel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job on the new command! Do try to update the test cases too! Please close this PR after reading the comments.

@@ -67,6 +67,26 @@ Examples:
* `add John Doe p/98765432 e/[email protected] a/John street, block 123, #01-01`
* `add Betsy Crowe pp/1234567 e/[email protected] pa/Newgate Prison t/criminal t/friend`

== Updating a person: `update`

Update an existing person in the address book. +

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Updates", remember to check for consistency in user guides because it is written by all your teammates, so there needs to be a uniform style of writing for the user guide to appear professional and neat. :)

}
public static final String MESSAGE_UPDATE_PERSON_SUCCESS = "Updated Person: %1$s";
public static final String MESSAGE_DUPLICATE_PERSON = "This person already exists in the address book";
@Override

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to add blank lines to your code, it improves readability.

final int targetIndex = parseArgsAsDisplayedIndex(index);
final Matcher matcher = PERSON_DATA_ARGS_FORMAT.matcher(rest.trim());
if (!matcher.matches()) {
return new IncorrectCommand(String.format(MESSAGE_INVALID_COMMAND_FORMAT, AddCommand.MESSAGE_USAGE));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using AddCommand's MESSAGE_USAGE, maybe try adding a separate "error message" to be more specific in the error handling process so when the system prints out the "message" you will be able to identify which exact part of your code it is running (instead of having the same message appear for 2 different situations).

@@ -174,6 +178,44 @@ private Command prepareDelete(String args) {
}
}

private Command prepareUpdate(String args) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing header comment. All non trivial methods should have javadoc format header comments.

@WuPeiHsuan
Copy link

Great enhancement! It is a useful command that can be used in our project haha

@shawn-t
Copy link

shawn-t commented Feb 11, 2019

Hey Guan Long, interesting enhancement! It saves users from the trouble of first deleting an existing entry and subsequently re-adding him/ her with the updated particulars! Good Job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants