[W5.7][T12-3]LIN XU #428
[W5.7][T12-3]LIN XU #428
Conversation
|
Hi @Woodnsuns, Your Github username is not recognized. Please post here. Note: this comment is posted by a bot. If you believe this is done in error, please create an issue at nus-se-pr-bot and add a link to this PR. |
nus-se-pr-bot
added
GithubUserNameRequested
tutorial.T12
team.3
and removed
GithubUserNameRequested
labels
There was a problem hiding this comment.
Good effort on this PR @Woodnsuns. In particular, good job on,
- updating docs
- updating I/O tests
Suggestions to improve further:
- update docs
- update with more I/O tests to test the new functionality
- 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.
| == Counting all persons: 'count' | ||
| Showing number of persons in the address book. | ||
|
|
||
| Format: 'count' |
There was a problem hiding this comment.
Good job updating the user doc
| */ | ||
| public class CountCommand extends Command { | ||
|
|
||
| public static final String COMMAND_WORD = "count"; |
There was a problem hiding this comment.
Try to avoid using public class member variables
| @@ -95,6 +95,7 @@ private void assertConstructingInvalidAddCmdThrowsException(String name, String | |||
| fail(error); | |||
| } | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Stray line break? It is a good practice to check the diff of the PR and fix any problems before submitting it.
| import seedu.addressbook.util.TestUtil; | ||
| import java.util.Collections; | ||
| import java.util.List; | ||
| public class CountCommandTest { |
There was a problem hiding this comment.
Coding standard requires to include descriptive header comments for public classes
| private static final List<ReadOnlyPerson> EMPTY_PERSON_LIST = Collections.emptyList(); | ||
| @Test | ||
|
|
||
| public void countCheckZero() throws Exception { |
There was a problem hiding this comment.
Coding standard requires to include descriptive header comments for public methods
| @@ -36,11 +36,14 @@ public void elementsAreUnique() throws Exception { | |||
| assertNotUnique(null, "a", "b", null); | |||
| } | |||
|
|
|||
|
|
|||
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.