[W5][T12-1]Sreyans Sipani #170
Conversation
|
Extended Find command for all fields |
|
Hi @sreycodes, your pull request title is invalid. For PR sent as submission of learning a topic/graded exercise, the PR name should be in the format of For team PR, the PR name should be in the format of 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. |
sreycodes
changed the title
|
@Tejas2805, can you help me review this PR? |
|
Very handy improvement. The search command is more powerful now |
| } else { | ||
| continue; | ||
| } | ||
| } |
There was a problem hiding this comment.
I think you should utilize guard clauses instead of nested if-else here
| @@ -2,6 +2,9 @@ | |||
|
|
|||
| import seedu.addressbook.data.exception.IllegalValueException; | |||
|
|
|||
| import java.util.Arrays; | |||
| import java.util.List; | |||
There was a problem hiding this comment.
I believe these should come before the seedu import :)
| */ | ||
| public List<String> getKeywordsInAddress() { | ||
| return Arrays.asList(value.split("[^a-zA-Z\\d]")); | ||
| } |
There was a problem hiding this comment.
For consistency with getTagValues perhaps this function should be in FindCommand.java file, or vice versa?
| @@ -2,6 +2,9 @@ | |||
|
|
|||
| import seedu.addressbook.data.exception.IllegalValueException; | |||
|
|
|||
| import java.util.Arrays; | |||
| import java.util.List; | |||
There was a problem hiding this comment.
These imports should come before as well
| @@ -33,8 +36,13 @@ public Email(String email, boolean isPrivate) throws IllegalValueException { | |||
| /** | |||
| * Returns true if the given string is a valid person email. | |||
| */ | |||
| public static boolean isValidEmail(String test) { | |||
| return test.matches(EMAIL_VALIDATION_REGEX); | |||
| public static boolean isValidEmail(String test) { return test.matches(EMAIL_VALIDATION_REGEX); } | |||
There was a problem hiding this comment.
This change is not related to this PR? Anyway the signature and body of a function should not be on the same line
| @@ -32,8 +34,14 @@ public Phone(String phone, boolean isPrivate) throws IllegalValueException { | |||
| /** | |||
| * Returns true if the given string is a valid person phone number. | |||
| */ | |||
| public static boolean isValidPhone(String test) { | |||
| return test.matches(PHONE_VALIDATION_REGEX); | |||
| public static boolean isValidPhone(String test) { return test.matches(PHONE_VALIDATION_REGEX); } | |||
| public boolean matches(Set<String> keywords) { | ||
| for(String keyword: keywords) { | ||
| if(isValidPhone(keyword) && value.indexOf(keyword) != -1) | ||
| return true; |
There was a problem hiding this comment.
This should be wrapped in braces :)
| @@ -14,9 +14,9 @@ | |||
| || Example: delete 1 | |||
| || Clears address book permanently. | |||
| || Example: clear | |||
| || find: Finds all persons whose names contain any of the specified keywords (case-sensitive) and displays them as a list with index numbers. | |||
| || find: Finds all persons whose names, emails, etc. contain any of the specified keywords (alphanumeric and case-sensitive) and displays them as a list withindex numbers. | |||
There was a problem hiding this comment.
withindex is missing a white space :)
| @@ -200,9 +200,9 @@ | |||
| || =================================================== | |||
| || Enter command: || [Command entered: find] | |||
| || Invalid command format! | |||
| || find: Finds all persons whose names contain any of the specified keywords (case-sensitive) and displays them as a list with index numbers. | |||
| || find: Finds all persons whose names, emails, etc. contain any of the specified keywords (alphanumeric and case-sensitive) and displays them as a list withindex numbers. | |||
| || =================================================== | ||
| || Enter command: || [Command entered: viewall 2] | ||
| || Person could not be found in address book | ||
| || The person index provided is invalid |
There was a problem hiding this comment.
Maybe you can update these tests since the added find command made then invalid?
No description provided.