-
Notifications
You must be signed in to change notification settings - Fork 6
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
Implement deleteBy ISBN or times and add tests #90
Implement deleteBy ISBN or times and add tests #90
Conversation
LGTM |
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
@@ -24,11 +24,18 @@ | |||
public static final String MESSAGE_DELETE_BOOK_SUCCESS = "Deleted Book: %1$s"; | |||
|
|||
private final String targetName; | |||
private final int attribute; |
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
/** | ||
* Parses input arguments and creates a new DeleteByCommand object | ||
*/ | ||
public class DeleteByCommandParser implements Parser<DeleteByCommand> { |
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. Interesting command with the usage of different types of keyword. Maybe there can also be the functionality to delete using more than 1 keyword at the same time.
|
||
import seedu.address.logic.commands.DeleteByCommand; | ||
|
||
|
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
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
boolean isTimesPresent = isPrefixesPresent(argMultimap, PREFIX_TIMES) | ||
&& !arePrefixesPresent(argMultimap, PREFIX_NAME, PREFIX_ISBN); | ||
|
||
|
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. The defensiveness seems to be present.
Implement deleteBy ISBN or times and add tests
Fix #52