[W5.7][T12-3]Park Sang Jun #422
Conversation
|
Looks good, but I wonder what caused the travis CI build to fail |
|
Looks nice, just a question. Did your IDE auto include [import seedu.addressbook.commands.*] instead of using whatever dedicated imports? A question that I wonders too as my IDE would sometimes remove dedicated imports and automatically include the parent import. |
|
Using .* to import all the commands is a good method to keep your code compact and easy to read. However, there is a possibility that the use of .* might result in unforeseen errors in the execution of the program |
There was a problem hiding this comment.
Good effort on this PR. In particular, good job on,
- updating user doc
- updating I/O and jUnit tests
Suggestions to improve further:
- update dev doc
- limit the PR to one enhancement and avoid including changes not related to that enhancement or unmerged code from other PRs
- pay closer attention to the coding standard
Please close the PR after viewing comments.
| @@ -176,6 +176,11 @@ Example: | |||
|
|
|||
| * `java seedu.addressbook.Main mydata.txt` | |||
|
|
|||
| == Counting the number of persons in the address book: `count` | |||
|
|
|||
| Counts the number of persons in the address book. + | |||
There was a problem hiding this comment.
Good job updating user doc
|
|
||
| public class CountCommand extends Command { | ||
|
|
||
| public static final String COMMAND_WORD = "count"; |
There was a problem hiding this comment.
Try avoiding public class member variables
| 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.
Coding standard does not permit wildcard imports
| @@ -43,4 +43,18 @@ private void assertAreUnique(Object... objects) { | |||
| private void assertNotUnique(Object... objects) { | |||
| assertFalse(Utils.elementsAreUnique(Arrays.asList(objects))); | |||
| } | |||
|
|
|||
| @Test | |||
There was a problem hiding this comment.
Coding standard requires to include descriptive header comments for public methods
| @@ -43,4 +43,18 @@ private void assertAreUnique(Object... objects) { | |||
| private void assertNotUnique(Object... objects) { | |||
| assertFalse(Utils.elementsAreUnique(Arrays.asList(objects))); | |||
| } | |||
|
|
|||
| @Test | |||
| public void isAnyNull() throws Exception { | |||
There was a problem hiding this comment.
This commit shouldn't be part of this PR. In general, a PR should not contain unmerged code from other PRs. Try to limit each PR to one enhancement, which means the PR should be based on a branch that starts off from the master branch that is in sync with the upstream master branch
No description provided.