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.11r][F14-B4] Liu Jianghao #757

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

Conversation

fishTT
Copy link

@fishTT fishTT commented Mar 5, 2018

added sort method
added sort to UserGuide and test

@nus-se-pr-bot
Copy link

Hi @fishTT, your pull request title is invalid.

For phase A, it should be in the format of [Learning Outcome ID][Team ID] Your Name, where [Learning Outcome ID] has no dashes or spaces (e.g. [W3.1a]) and [Team ID] has one dash only and no spaces (e.g. [W14-A2] means Wednesday 2pm (14 hrs), Phase A, Team 2).

For phase B, it should be in the format of [v1.x][Team ID] Product Name.

Please follow the instructions given strictly and edit your title for reprocessing.

Submit only one learning outcome per pull request (unless otherwise stated in instructions) and do remember to create your branches from the commit where the master branch is pointing at so that each PR you submit only consist of commits meant for the activity.

Note: this comment is posted by a bot. If you believe this is done in error, please create an issue at cs2103-pr-bot and add a link to this PR.

@fishTT fishTT changed the title W5.11r [W5.11r][F14-B4] Liu Jianghao Mar 5, 2018
@fishTT
Copy link
Author

fishTT commented Mar 5, 2018

edited title for reprocessing.

Copy link

@liuyang1213 liuyang1213 left a comment

Choose a reason for hiding this comment

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

Good implementation. But you should read the coding standard more carefully and format your code correctly. Also, please update the test cases and documentation :)

@@ -16,6 +16,7 @@
+ "\n" + ClearCommand.MESSAGE_USAGE
+ "\n" + FindCommand.MESSAGE_USAGE
+ "\n" + ListCommand.MESSAGE_USAGE
+ "\n" + SortCommand.MESSAGE_USAGE

Choose a reason for hiding this comment

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

Coding standard violation!

*/
public void sort() {
internalList.sort(
Comparator.comparing((Person p) -> p.getName().toString(),

Choose a reason for hiding this comment

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

Indentation of this line should be 8 more spaces than the previous line.

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.

4 participants