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][T12-3]Tan Kian Wei Jason #167

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

Conversation

jtankw3
Copy link

@jtankw3 jtankw3 commented Feb 12, 2019

Added sort command which sorts the addressbook in alphabetical order.
Also updated tests and user documentation to include sort command.

Copy link

@flxffy flxffy left a comment

Choose a reason for hiding this comment

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

Very handy command. Code is concise and coding style is coherent with the rest of the files. Perhaps include the option to sort by different comparators, like last name, first name, email address, etc., instead of just the default full name.

Copy link

@melpulomas melpulomas left a comment

Choose a reason for hiding this comment

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

Useful addition to the addressbook. Able to understand the code written clearly as it follows the same coding style. You could implement a way to enable the user to sort the list automatically whenever an additional input have been made for the next time.

Copy link

@nhs-work nhs-work left a comment

Choose a reason for hiding this comment

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

Great job on your first PR! Please close the PR after you have read my comments.

@@ -72,6 +72,11 @@ Examples:
Shows a list of all persons, along with their non-private details, in the address book. +
Format: `list`

== Listing all persons : `sort`

Sorts the address book in alphabetical order. +

Choose a reason for hiding this comment

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

It is unclear which field of the address book is being sorted.

@@ -73,33 +74,36 @@ public Command parseCommand(String userInput) {

switch (commandWord) {

case AddCommand.COMMAND_WORD:
return prepareAdd(arguments);
case AddCommand.COMMAND_WORD:

Choose a reason for hiding this comment

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

Note that there should not be indentation for case clauses.

/**
* Asserts that the addressbook has been successfully sorted.
*
* The addressBook passed in will not be modified (no side effects).

Choose a reason for hiding this comment

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

Are you sure that sorting the address book does not result in a different ordering of the persons?

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