-
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
Extend functionality to Find Command to search in specific Role Lists #89
Conversation
Support find command filtering separately for volunteers and befriendees. Added testcases to test for find by role.
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.
Overall, LGTM, may need some minor refactoring to be line with our latest design changes.
if (searchVolunteer && searchBefriendee) { | ||
model.updateFilteredPersonList(predicate); | ||
return new CommandResult( | ||
String.format(Messages.MESSAGE_PERSONS_LISTED_OVERVIEW, model.getFilteredPersonList().size())); | ||
|
||
} else if (searchVolunteer) { | ||
model.updateFilteredVolunteerList(predicate); | ||
return new CommandResult( | ||
String.format(Messages.MESSAGE_PERSONS_LISTED_OVERVIEW_WITH_ROLE, | ||
model.getFilteredVolunteerList().size(), | ||
"volunteer")); | ||
|
||
} else if (searchBefriendee) { | ||
model.updateFilteredBefriendeeList(predicate); | ||
return new CommandResult( | ||
String.format(Messages.MESSAGE_PERSONS_LISTED_OVERVIEW_WITH_ROLE, | ||
model.getFilteredBefriendeeList().size(), | ||
"befriendee")); | ||
} |
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.
Really like the avoidance of deep nesting here.
boolean searchVolunteer = false; | ||
boolean searchBefriendee = false; |
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.
Could perhaps rename these boolean variables to be more boolean sounding
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.
Noted on this
/** | ||
* Updates the filter of the filtered volunteer list to filter by the given {@code predicate}. | ||
* Befriendee list is not filtered. | ||
* @throws NullPointerException if {@code predicate} is null. | ||
*/ | ||
void updateFilteredVolunteerList(Predicate<Person> predicate); | ||
|
||
/** | ||
* Updates the filter of the filtered befriendee list to filter by the given {@code predicate}. | ||
* Volunteer list is not filtered. | ||
* @throws NullPointerException if {@code predicate} is null. | ||
*/ | ||
void updateFilteredBefriendeeList(Predicate<Person> predicate); |
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.
Not sure if these commands have been phased out through jia en's latest pr, may have to refactor this.
filteredBefriendees.setPredicate(person -> predicate.test(person) && person.isBefriendee()); | ||
} | ||
|
||
@Override | ||
public void updateFilteredVolunteerList(Predicate<Person> predicate) { | ||
requireNonNull(predicate); | ||
|
||
// Only apply predicate to volunteers | ||
filteredPersons.setPredicate(person -> person.isBefriendee() || predicate.test(person)); | ||
filteredVolunteers.setPredicate(person -> predicate.test(person) && person.isVolunteer()); | ||
} | ||
|
||
@Override | ||
public void updateFilteredBefriendeeList(Predicate<Person> predicate) { | ||
requireNonNull(predicate); | ||
|
||
// Only apply predicate to befriendees | ||
filteredPersons.setPredicate(person -> predicate.test(person) || person.isVolunteer()); | ||
filteredBefriendees.setPredicate(person -> predicate.test(person) && person.isBefriendee()); |
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.
May have to move this functionality down into the Datastore structure
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.
Done! Thank you for the insight!
Resolve Merge Conflicts resulting from refactoring of storage
Rename booleans for better clarity
Support find command filtering separately for volunteers and befriendees.
Added testcases to test for find by role.
Current Format:
find KEYWORD [MORE_KEYWORDS]
Added feature: Filter separately for volunteer and befriendee, while not clearing the other list. This is to faciliate pairing and unpairing operations using the index.
find r/volunteer KEYWORD [MORE_KEYWORDS]
find r/befriendee KEYWORD [MORE_KEYWORDS]
Action: Will find given person in the list of the respective role, and leave other list untouched.
Closes #74