-
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
Update Find Command #83
Update Find Command #83
Conversation
jonahtanjz
commented
Oct 20, 2020
•
edited
edited
- Tokenize Find command
- Make each prefix in Find command optional
- Add ExpirySearchPredicate, PrioritySearchPredicate and TagSearchPredicate
- Parse arguments in Find command to generate the respective Predicates
- Add JUnit tests for new Find command feature
- Add JUnit tests for ExpirySearchPredicate, PrioritySearchPredicate and TagSearchPredicate
…ExpiryDateSearchPredicate and PrioritySearchPredicate
Codecov Report
@@ Coverage Diff @@
## master #83 +/- ##
============================================
+ Coverage 71.62% 72.08% +0.46%
- Complexity 398 435 +37
============================================
Files 69 72 +3
Lines 1237 1322 +85
Branches 121 144 +23
============================================
+ Hits 886 953 +67
Misses 319 319
- Partials 32 50 +18
Continue to review full report at Codecov.
|
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.
The changes look great! I left a few comments below.
return tagsToFind.stream() | ||
.anyMatch(tagToFind -> { | ||
for (Tag tag : food.getTags()) { | ||
if (StringUtil.containsWordIgnoreCase(tag.tagName, tagToFind.tagName)) { |
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.
Since a single tag can have multiple words, so containsWordIgnoreCase will throw an error if its argument has multiple words?
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.
Yup good catch! Fixed
* Tests that a {@code Food}'s {@code Priority} matches the search given. | ||
*/ | ||
public class ExpiryDateSearchPredicate implements Predicate<Food> { | ||
private static Logger logger = Logger.getLogger("ExpiryDateSearchPredicate.class"); |
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.
Should this be LogsCenter.getLogger(ExpiryDateSearchPredicate.Class) instead?
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.
Alright changed.
@@ -57,28 +89,142 @@ public void equals() { | |||
@Test | |||
public void execute_zeroKeywords_noFoodFound() { |
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.
Very minor, but for consistency, do you think this should be execute_zeroDescriptionKeywords_noFoodFound()?
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.
Changed. Thanks
} | ||
|
||
@Test | ||
public void execute_multipleTagsSearch_multipleFoodsFound() { |
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.
Should we include tags with spaces as well?
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.
Alright. Added new tests
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