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][M11-3] Dannica Ong #28

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

Conversation

dannong
Copy link

@dannong dannong commented Mar 2, 2019

  • Displays the list of contacts after each addition of new contact
  • Edit Help page for easier reading
  • Updated User Guide
  • Added new Unit tests
  • Update existing tests

@dannong dannong changed the title [W6][M11-3] Dannica Ong [W7][M11-3] Dannica Ong Mar 2, 2019
@nus-se-pr-bot nus-se-pr-bot requested a review from stephlewyh March 2, 2019 18:24
Copy link

@devamanyu devamanyu left a comment

Choose a reason for hiding this comment

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

Good PR. If I remember correctly, wasn't your 2kloc PR same feature? Has there been any improvement over that feature?

Please read some minor comments that I have added and then close this PR.

Command.getMessageForPersonListShownSummary(expectedList),
expectedAB,
true,
expectedList);

Choose a reason for hiding this comment

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

Good job in testing this scenario. Try to see if other exception cases can be tested too.

@@ -60,7 +61,9 @@ public ReadOnlyPerson getPerson() {
public CommandResult execute() {
try {
addressBook.addPerson(toAdd);
return new CommandResult(String.format(MESSAGE_SUCCESS, toAdd));
List<ReadOnlyPerson> allPersons = addressBook.getAllPersons().immutableListView();
//return new CommandResult(getMessageForPersonListShownSummary(allPersons), allPersons);

Choose a reason for hiding this comment

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

Please remove unwanted code lines instead of leaving them as commented out chunks.

devamanyu
devamanyu approved these changes Mar 8, 2019
@dannong
Copy link
Author

dannong commented Mar 8, 2019 via email

@devamanyu
Copy link

Oh great!

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