-
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
[Defensive] FindCommand #180
[Defensive] FindCommand #180
Conversation
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, just some comments!
@@ -20,6 +20,7 @@ | |||
public static final String MESSAGE_ACTIVITIES_LISTED_OVERVIEW = "%1$d activities listed!"; | |||
public static final String MESSAGE_ACCOMMODATIONS_LISTED_OVERVIEW = "%1$d accommodations listed!"; | |||
public static final String MESSAGE_FRIENDS_LISTED_OVERVIEW = "%1$d friends listed!"; | |||
public static final String MESSAGE_TRAVELPLANS_LISTED_OVERVIEW = "%1$d travel plans listed!"; | |||
public static final String WRONG_DIRECTORY = "You are currently in the wishlist which can only find activity.\n" | |||
+ "Please goto to a travelplan to edit friend, activity or accommodation"; |
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.
Is this a clearer phrasing?
"Please go to your desired travelplan to edit friend, activity or accommodation, using the goto command.";
|
||
private final NameContainsKeywordsPredicate predicate; | ||
private final int travelPlanObjectType; | ||
|
||
/** | ||
* Constructor for FindCommand | ||
* | ||
* @param predicate contains a list of strings to filter travel plan object list | ||
* @param predicate contains a list of strings to filter travel plan object list |
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.
I think you accidentally added a big space here haha
if (this.travelPlanObjectType == ParserUtil.ACTIVITY_INDEX) { | ||
model.updateFilteredActivityList(predicate); | ||
message = MESSAGE_ACTIVITIES_LISTED_OVERVIEW; | ||
size = model.getFilteredActivityList().size(); | ||
|
||
} else if (this.travelPlanObjectType == 1) { //accommodation | ||
} else if (this.travelPlanObjectType == ParserUtil.ACCOMMODATION_INDEX) { | ||
model.updateFilteredAccommodationList(predicate); | ||
message = MESSAGE_ACCOMMODATIONS_LISTED_OVERVIEW; | ||
size = model.getFilteredAccommodationList().size(); | ||
|
||
|
||
} else if (this.travelPlanObjectType == 2) { //friend | ||
} else if (this.travelPlanObjectType == ParserUtil.FRIEND_INDEX) { | ||
model.updateFilteredFriendList(predicate); | ||
message = MESSAGE_FRIENDS_LISTED_OVERVIEW; | ||
size = model.getFilteredFriendList().size(); | ||
|
||
} else { | ||
assert 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.
Can use switch case but not a big problem.
|
||
private final NameContainsKeywordsPredicate predicate; | ||
private final int travelPlanObjectType; | ||
|
||
/** | ||
* Constructor for FindCommand | ||
* | ||
* @param predicate contains a list of strings to filter travel plan object list | ||
* @param predicate contains a list of strings to filter travel plan object list |
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.
Why is there a huge whitespace here tho?
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
Include check to ensure wishlist can only find activity object
Assert valid inputs
close #163