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

Added sort command #33

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

Added sort command #33

wants to merge 4 commits into from

Conversation

sreycodes
Copy link

Copy link

@bqnguyen94 bqnguyen94 left a comment

Choose a reason for hiding this comment

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

Looks solid 👍 for the tests, it would be nice to change the data so that different sort arguments give different results.

@@ -579,6 +586,35 @@ private static String executeListAllPersonsInAddressBook() {
return getMessageForPersonsDisplayedSummary(toBeDisplayed);
}

/**
* Displays all persons in the address book to the user; in added order.

Choose a reason for hiding this comment

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

I think this comment is not correct?

switch(rawArgs.trim()) {
case "name": return PERSON_DATA_INDEX_NAME;
case "phone": return PERSON_DATA_INDEX_PHONE;
case "email": return PERSON_DATA_INDEX_EMAIL;

Choose a reason for hiding this comment

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

Is it better to put these case strings as constants?

return name.matches("(\\w|\\s)+"); // name is nonempty mixture of alphabets and whitespace
//TODO: implement a more permissive validation
// name is nonempty mixture of alphabets and whitespace and hyphens
return name.matches("(\\w|\\s|-)+");

Choose a reason for hiding this comment

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

Hmm this doesn't seem to be related to sort command? If so, it should have its own PR.

return phone.matches("\\d+"); // phone nonempty sequence of digits
//TODO: implement a more permissive validation
// phone nonempty sequence of digits and hyphens with an optional plus in the front
return phone.matches("\\+*(\\d+|-)+");

Choose a reason for hiding this comment

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

Same here.

@bqnguyen94
Copy link

@sreycodes may I know which exercise this PR is for? Is it the W3.3c branching and 1KLoC of week 3 or 2KLoC of week 5?

@nus-se-pr-bot
Copy link

Hi @sreycodes, your pull request title is invalid.

For PR sent to satisfy a learning outcome, the PR name 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-2] means Wednesday 2pm (14 hrs), Team 2).
E.g. If you are in tutorial W09 (i.e. Wednesday 9am), team 1, and is submitting a PR for LO W2.2b, the PR title would be [W2.2b][W09-1] James Yong. Note that your tutorial IDs are different from those shown in CORS/IVLE.

For team PR, the PR name should be in the format of [Team ID] Product Name (e.g. [T09-2] Contact List Pro).

Please follow the above format strictly and edit your title for reprocessing.

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

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