[W5] [T12-4] Jeremy Yiren Low #171
Conversation
…ASSED test show as FAIL
…mains that you can only undo the previous command.
…eters to cover loopholes found from testing.
|
Very interesting feature! In fact, this might be the most creative enhancement I've ever seen. Bravo! #LATB |
|
Here's my two cents on the commit message: try to keep it short (<50 char) & the title should be in present tense e.g. "Add...", "Update...", "Change...". For additional information, put it in a paragraph instead of the title commit message. Here's some useful links to learn more about writing a good commit message: #LATB |
There was a problem hiding this comment.
Fantastic job on your first PR! You have managed to demonstrate a great understanding of the addressbook. Unfortunately, you forgot to update the User Guide and JUnit tests. Make sure to always update documentation and all tests whenever you make changes. Keep up the good work! 👍
| @@ -32,6 +33,8 @@ | |||
| /** The list of person shown to the user most recently. */ | |||
| private List<? extends ReadOnlyPerson> lastShownList = Collections.emptyList(); | |||
|
|
|||
| /** The previous command entered by user. */ | |||
| private String PreviousUserCommand = Messages.HISTORY_EMPTY; | |||
| + "\n" + ViewCommand.MESSAGE_USAGE | ||
| + "\n" + ViewAllCommand.MESSAGE_USAGE | ||
| + "\n" + HelpCommand.MESSAGE_USAGE | ||
| + "\n" + ExitCommand.MESSAGE_USAGE | ||
| ); | ||
| , true); |
There was a problem hiding this comment.
Note that you should break after a comma and that this line should be indented as well.
| try { | ||
| addressBook.removeLastPerson(); | ||
| } | ||
| catch (UniquePersonList.PersonNotFoundException pnfe) { |
There was a problem hiding this comment.
Catch statements should be on the same line as the closed brace of the previous try/catch closed brace.
| } catch (IndexOutOfBoundsException ie) { | ||
| return new CommandResult(Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX); | ||
| return new CommandResult(Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX, true); |
There was a problem hiding this comment.
Shouldn't this return false as the second parameter?
| */ | ||
| public void removeLastPerson() throws PersonNotFoundException { | ||
| Iterator<Person> itr = allPersons.iterator(); | ||
| assert itr.hasNext(); |
There was a problem hiding this comment.
Great use of assertions here to verify the program's state!
| import java.util.Collections; | ||
| import java.util.HashSet; | ||
| import java.util.Set; | ||
| import java.util.*; |
There was a problem hiding this comment.
Imported classes should always be listed explicitly.
| import seedu.addressbook.commands.ListCommand; | ||
| import seedu.addressbook.commands.ViewAllCommand; | ||
| import seedu.addressbook.commands.ViewCommand; | ||
| import seedu.addressbook.commands.*; |
There was a problem hiding this comment.
Imported classes should always be listed explicitly.
Summary:
Simple undo that only works on the previous command.
Works with add or delete commands only (wanted to do list but I need a break..)