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][T09-2]Cheah Zhi Kang #26

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

Conversation

cheahzk
Copy link

@cheahzk cheahzk commented Feb 10, 2019

Added sort command to sort all persons' full names alphabetically and display them in a list.
Able to detect correct sort command.

Credits to Marcus Phua for some of the codes in mine.

@Wanchunwei
Copy link

Good feature! It is a useful and convenient function for customers.

@ghost
Copy link

ghost commented Feb 10, 2019

It looks good when listing all people after adding sort function.

@jingchen-z
Copy link

Very useful functionalities, which makes the output more ordered and organized. :)

Copy link

@sanjukta99 sanjukta99 left a comment

Choose a reason for hiding this comment

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

You can consider abstracting your Sort command further so that it only sorts the list of persons in the Address Book but does not list them (following the Single Responsibility Principle). Overall, good job on creating a useful feature, with updates to the UG and test cases! Please read the reviews and close this PR.

docs/UserGuide.adoc Show resolved Hide resolved
test/expected.txt Show resolved Hide resolved
@@ -50,6 +51,7 @@
list
add Esther Potato p/555555 e/[email protected] pa/555, epsilon street t/tubers t/starchy
list
sort

Choose a reason for hiding this comment

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

Perhaps your input for the test case should not be added in a lexicographically sorted manner? You can consider changing the order of one or more of the test inputs to verify whether your command works as expected.

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.

6 participants