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.7][F11-4]Ng Yuan Yun Nigel #445

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

Conversation

nigelngyy
Copy link

No description provided.

@@ -72,7 +72,8 @@ public UniquePersonList getAllPersons() {
* Returns a new UniquePersonList of all persons in the address book at the time of the call sorted in alphabetical order.
*/
public UniquePersonList getSortedAllPersons() {
return new UniquePersonList(allPersons).sort();
UniquePersonList toSort = new UniquePersonList(allPersons);
return toSort.sort();

Choose a reason for hiding this comment

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

Doesn't update the main list of persons? Only sort the new list created (the copy of the main list)?

@@ -160,7 +165,7 @@ Format: `clear`

Exits the program. +
Format: `exit`

l

Choose a reason for hiding this comment

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

There is the extra "l" which may have affected the presentation, and GitHub's ability to session the paragraph properly

Copy link

@jelneo jelneo left a comment

Choose a reason for hiding this comment

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

It is encouraged to update the JUnit tests if you have not done so. Please close this PR after reading my comments.

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