-
Notifications
You must be signed in to change notification settings - Fork 35
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
[W5][T09-3] Yow Ren Jie #46
base: master
Are you sure you want to change the base?
Conversation
@@ -28,7 +28,7 @@ | |||
private static final List<ReadOnlyPerson> EMPTY_PERSON_LIST = Collections.emptyList(); | |||
private static final Set<String> EMPTY_STRING_SET = Collections.emptySet(); | |||
|
|||
@Test | |||
@org.junit.jupiter.api.Test |
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.
What does this change do?
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.
@gautamrajulu how do you declare that a method is a test method?
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.
@ren jie why do you need a new junit framework for this enhancement?
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.
@arhjaye why do you need another junit test framework for your enhancement?
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.
Hi Ren Jie, good job in doing the following:
(1) Updated user doc
(2) Updated tests
However, do remember to add header comments when you add new enhancements.
You may close this PR now.
public static final String MESSAGE_USAGE = COMMAND_WORD + ": prints goodbye message.\n" | ||
+ "Example: " + COMMAND_WORD; | ||
|
||
@Override |
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.
Missing header comment. All non-trivial methods (in this case your enhancement functionality) should have java doc format header comments.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imported classes should always be listed explicitly. E.g. java.util.List
and not java.util.*
, which does not follow NUS Java coding standards.
@@ -73,6 +63,9 @@ public Command parseCommand(String userInput) { | |||
|
|||
switch (commandWord) { | |||
|
|||
case GoodbyeCommand.COMMAND_WORD: |
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.
Unnecessary indentation here.
@@ -22,7 +22,8 @@ public CommandResult execute() { | |||
+ "\n" + ViewCommand.MESSAGE_USAGE | |||
+ "\n" + ViewAllCommand.MESSAGE_USAGE | |||
+ "\n" + HelpCommand.MESSAGE_USAGE | |||
+ "\n" + ExitCommand.MESSAGE_USAGE | |||
+ "\n" + GoodbyeCommand.MESSAGE_USAGE |
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.
Unnecessary indentation. Reduces code readability.
@@ -28,7 +28,7 @@ | |||
private static final List<ReadOnlyPerson> EMPTY_PERSON_LIST = Collections.emptyList(); | |||
private static final Set<String> EMPTY_STRING_SET = Collections.emptySet(); | |||
|
|||
@Test | |||
@org.junit.jupiter.api.Test |
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.
@gautamrajulu how do you declare that a method is a test method?
@@ -28,7 +28,7 @@ | |||
private static final List<ReadOnlyPerson> EMPTY_PERSON_LIST = Collections.emptyList(); | |||
private static final Set<String> EMPTY_STRING_SET = Collections.emptySet(); | |||
|
|||
@Test | |||
@org.junit.jupiter.api.Test | |||
public void addCommand_invalidName_throwsException() { |
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.
Missing header comment. All non-trivial methods should have java doc format header comments.
@@ -28,7 +28,7 @@ | |||
private static final List<ReadOnlyPerson> EMPTY_PERSON_LIST = Collections.emptyList(); | |||
private static final Set<String> EMPTY_STRING_SET = Collections.emptySet(); | |||
|
|||
@Test | |||
@org.junit.jupiter.api.Test |
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.
@ren jie why do you need a new junit framework for this enhancement?
@@ -28,7 +28,7 @@ | |||
private static final List<ReadOnlyPerson> EMPTY_PERSON_LIST = Collections.emptyList(); | |||
private static final Set<String> EMPTY_STRING_SET = Collections.emptySet(); | |||
|
|||
@Test | |||
@org.junit.jupiter.api.Test |
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.
@arhjaye why do you need another junit test framework for your enhancement?
No description provided.