Skip to content
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

Update student related commands #56

Merged

Conversation

ypinhsuan
Copy link

Resolves #11

Copy link

@samlsm samlsm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍

Comment on lines 30 to 31
* Contains integration tests (interaction with the Model, UndoCommand and RedoCommand) and unit tests for
* EditStudentCommand.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it might be good to follow the same format as DeleteStudentCommandTest to be consistent?

Suggested change
* Contains integration tests (interaction with the Model, UndoCommand and RedoCommand) and unit tests for
* EditStudentCommand.
* Contains integration tests (interaction with the Model, UndoCommand and RedoCommand) and unit tests for
* {@code EditStudentCommand}.

public static final String COMMAND_WORD = "clear";
public static final String MESSAGE_SUCCESS = "Tutor's Pet has been cleared!";
public static final String COMMAND_WORD = "clear-student";
public static final String MESSAGE_SUCCESS = "All students in Tutor's Pet has been cleared!";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how you specify that clear removes all students!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public static final String MESSAGE_SUCCESS = "All students in Tutor's Pet has been cleared!";
public static final String MESSAGE_SUCCESS = "All students in Tutor's Pet have been cleared!";

@junlong4321 junlong4321 added this to the v1.2 milestone Sep 24, 2020
Copy link

@dextertanyj dextertanyj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some minor suggestions in terms of test case naming, otherwise everything else looks good to me!

public static final String COMMAND_WORD = "clear";
public static final String MESSAGE_SUCCESS = "Tutor's Pet has been cleared!";
public static final String COMMAND_WORD = "clear-student";
public static final String MESSAGE_SUCCESS = "All students in Tutor's Pet has been cleared!";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public static final String MESSAGE_SUCCESS = "All students in Tutor's Pet has been cleared!";
public static final String MESSAGE_SUCCESS = "All students in Tutor's Pet have been cleared!";

@@ -21,7 +21,7 @@
* Returns an add command string for adding the {@code student}.
*/
public static String getAddCommand(Student student) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we can rename this to getAddStudentCommand(Student student)

assertEquals(new DeleteCommand(INDEX_FIRST_STUDENT), command);
DeleteStudentCommand command = (DeleteStudentCommand) parser.parseCommand(
DeleteStudentCommand.COMMAND_WORD + " " + INDEX_FIRST_STUDENT.getOneBased());
assertEquals(new DeleteStudentCommand(INDEX_FIRST_STUDENT), command);
}

@Test
public void parseCommand_edit() throws Exception {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public void parseCommand_edit() throws Exception {
public void parseCommand_editStudent() throws Exception {

assertTrue(parser.parseCommand(ClearCommand.COMMAND_WORD) instanceof ClearCommand);
assertTrue(parser.parseCommand(ClearCommand.COMMAND_WORD + " 3") instanceof ClearCommand);
assertTrue(parser.parseCommand(ClearStudentCommand.COMMAND_WORD) instanceof ClearStudentCommand);
assertTrue(parser.parseCommand(ClearStudentCommand.COMMAND_WORD + " 3") instanceof ClearStudentCommand);
}

@Test
public void parseCommand_delete() throws Exception {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public void parseCommand_delete() throws Exception {
public void parseCommand_deleteStudent() throws Exception {

@@ -36,30 +36,30 @@
@Test
public void parseCommand_add() throws Exception {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public void parseCommand_add() throws Exception {
public void parseCommand_addStudent() throws Exception {

AddCommand command = (AddCommand) parser.parseCommand(StudentUtil.getAddCommand(student));
assertEquals(new AddCommand(student), command);
AddStudentCommand command = (AddStudentCommand) parser.parseCommand(StudentUtil.getAddCommand(student));
assertEquals(new AddStudentCommand(student), command);
}

@Test
public void parseCommand_clear() throws Exception {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public void parseCommand_clear() throws Exception {
public void parseCommand_clearStudent() throws Exception {

@@ -71,9 +71,9 @@ public void parseCommand_exit() throws Exception {
@Test
public void parseCommand_find() throws Exception {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public void parseCommand_find() throws Exception {
public void parseCommand_findStudent() throws Exception {


@Test
public void parse_emptyArg_throwsParseException() {
assertParseFailure(parser, " ", String.format(MESSAGE_INVALID_COMMAND_FORMAT, FindCommand.MESSAGE_USAGE));
assertParseFailure(parser, " ", String.format(MESSAGE_INVALID_COMMAND_FORMAT,
FindStudentCommand.MESSAGE_USAGE));
}

@Test
public void parse_validArgs_returnsFindCommand() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public void parse_validArgs_returnsFindCommand() {
public void parse_validArgs_returnsFindStudentCommand() {


private DeleteCommandParser parser = new DeleteCommandParser();
private DeleteStudentCommandParser parser = new DeleteStudentCommandParser();

@Test
public void parse_validArgs_returnsDeleteCommand() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public void parse_validArgs_returnsDeleteCommand() {
public void parse_validArgs_returnsDeleteStudentCommand() {

@ypinhsuan ypinhsuan merged commit 8df9335 into AY2021S1-CS2103T-T10-4:master Sep 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update command keywords for student related actions for v1.2
4 participants