Add basic functionalities for the ClearAllCommand #45
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Todo (maybe enhancement later): Dislay that the list is empty after the clear command, i.e Showing a prompt saying the list is empty
Codecov Report
@@ Coverage Diff @@
## master #45 +/- ##
============================================
- Coverage 71.33% 70.65% -0.69%
Complexity 417 417
============================================
Files 74 75 +1
Lines 1284 1295 +11
Branches 131 132 +1
============================================
- Hits 916 915 -1
- Misses 334 345 +11
- Partials 34 35 +1
Continue to review full report at Codecov.
|
yongxiangng
reviewed
Oct 10, 2021
Comment on lines
17
to
19
| public static final String MESSAGE_USAGE = COMMAND_WORD | ||
| + ": Clears all entries from the address book.\n" | ||
| + "Example: " + COMMAND_WORD + " 1"; |
There was a problem hiding this comment.
- Address book to ForYourInterest
- " 1" is not needed, see user guide here for the format
Comment on lines
76
to
78
| case ClearAllCommand.COMMAND_WORD: | ||
| return new ClearAllCommand(); | ||
|
|
There was a problem hiding this comment.
Place this in alphabetical order if possible, in the same order as the import statements
Comment on lines
96
to
102
| /** | ||
| * Clears every entry from this {@code AddressBook}. | ||
| */ | ||
| public void clearAllPerson() { | ||
| persons.clearAllPerson(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Not sure if we should change this @code AddressBook part, we might be able to refactor this all at once in the future
Comment on lines
27
to
37
| @Override | ||
| public CommandResult execute(Model model) throws CommandException { | ||
| List<Person> lastShownList = model.getFilteredPersonList(); | ||
|
|
||
| if (lastShownList.size() == 0) { | ||
| throw new CommandException(Messages.MESSAGE_EMPTY_LIST); | ||
| } | ||
|
|
||
| model.clearAllPerson(); | ||
| return new CommandResult(MESSAGE_CLEAR_ALL_SUCCESS); | ||
| } |
There was a problem hiding this comment.
Try to get the unfiltered list.
Using the filtered list may lead to this scenario:
- There is data in the JSON file
- User filters with
findcommand and uses some nonsense keyword - User gets 0 search results back
- lastShownList.size() == 0
- Exception thrown saying the JSON file is empty when it is not empty
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Todo (maybe enhancement later): Dislay that the list is empty after the clear command,
i.e Showing a prompt saying the list is empty