-
Notifications
You must be signed in to change notification settings - Fork 5
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
Branch update find command format #69
Branch update find command format #69
Conversation
original was /<tag> now is <tag>/ To make it in uniform with the other commands
Include additional test cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! This should:
- establish the correct find command format
- implement a less strict find protocol for all 3 FindCommands
@@ -17,7 +16,7 @@ public ContactContainsKeywordsPredicate(List<String> keywords) { | |||
@Override | |||
public boolean test(Person person) { | |||
return this.getKeywords().stream() | |||
.anyMatch(keyword -> StringUtil.containsWordIgnoreCase(person.getPhone().value, keyword)); | |||
.anyMatch(keyword -> person.getPhone().value.toLowerCase().contains(keyword.toLowerCase())); | |||
} | |||
|
|||
@Override |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can keep in view to possibly abstract the test
method into the superclass FindCommand if we decide to extend the logic
return new FindByContactCommand( | ||
new ContactContainsKeywordsPredicate(Arrays.asList(searchTermArray))); | ||
case "/e": | ||
case "e/": | ||
return new FindByEmailCommand( | ||
new EmailContainsKeywordsPredicate(Arrays.asList(searchTermArray))); | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can keep in view to shorten the code here by establishing Arrays.asList(....) outside the switch so we do not have to type this 3 times
Update find command tag from '/' to '/'
Improved finding to include results with partial matches
Include additional test cases
Closes #63