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

[W7][T11-3]Jacob Han #50

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jacobhan
Copy link

@jacobhan jacobhan commented Mar 5, 2019

No description provided.

@jacobhan jacobhan changed the title [W2.2b][T11-3]Jacob Han [W7][T11-3]Jacob Han Mar 5, 2019
@nus-se-pr-bot nus-se-pr-bot requested a review from stephlewyh March 5, 2019 04:02
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.

Overall good attempt! You may close this PR after reading the comments 👍

randomPerson.add(random);
return new CommandResult(MESSAGE_SUCCESS, randomPerson);
} else {
return new CommandResult(MESSAGE_FAILURE);

Choose a reason for hiding this comment

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

Good job on checking for this event! Very user-friendly.

new Email(Email.EXAMPLE, false),
new Address(Address.EXAMPLE, true),
new HashSet<>(Arrays.asList(new Tag("tag1"), new Tag("tag2"), new Tag("tag3")))
new Name(Name.EXAMPLE),

Choose a reason for hiding this comment

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

Try to follow the checkstyle guidelines, indentation error here.

yingrong1996 pushed a commit to yingrong1996/addressbook-level3 that referenced this pull request Mar 27, 2019
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.

3 participants