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

Add ViewCommand and ViewCommandParser #91

Merged

Conversation

ZhangWanlin98
Copy link
Collaborator

Added ViewCommand, ViewCommandParser and DateMatchesPredicate classes and related tests.

…into AY2021S1-CS2103T-T12-4-master

# Conflicts:
#	src/test/java/seedu/address/logic/commands/AssignCommandTest.java
#	src/test/java/seedu/address/testutil/TypicalAppointments.java
@ZhangWanlin98 ZhangWanlin98 linked an issue Oct 12, 2020 that may be closed by this pull request
Copy link
Collaborator

@YangYue128-helen YangYue128-helen left a comment

Choose a reason for hiding this comment

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

I think Nuudle should support the "view" command without a date predicate. Other than this, LGTM!

Comment on lines +28 to +31
if (argMultimap.getValue(PREFIX_DATE).isEmpty()) {
throw new ParseException(
String.format(MESSAGE_INVALID_COMMAND_FORMAT, ViewCommand.MESSAGE_USAGE)
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shall we consider the situation when a nurse wants to view the whole appointment list (without a date specified)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that he handled this under NuudleParser but I think it would be better to do the argument checking in this file instead?
Maybe can try doing the isEmpty check before tokenize?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, as @YangYue128-helen mentioned, perhaps it'll be ok to leave out this check as we are able to support view commands without a DATE predicate as well?

Based on line 53 of ViewCommand.java, the current system seems to support the display of all future appointments if the predicate input is null (in the case that a date is not entered by the user) so I think you've got it covered @ZhangWanlin98 👍🏻

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh wait after reviewing NuudleParser as mentioned by @JinHao-L , I think the check done here is actually valid as the ViewCommandParser is only instantiated if extra input by the user following the view command is detected. Hence, it actually indicates an invalid command format if PREFIX_DATE is empty as it would mean that something else is written.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hence, it actually indicates an invalid command format if PREFIX_DATE is empty as it would mean that something else is written

@Avalionnet yup, I covered empty arg in the switch cases in NuudleParser, which is just list other one word command
but now I have changed to cover it in the ViewCommandParser
I think both ways are good, what yall think?

Copy link
Collaborator

@JinHao-L JinHao-L left a comment

Choose a reason for hiding this comment

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

Other than the above-mentioned comment, I think it LGTM! 👍

Comment on lines +28 to +31
if (argMultimap.getValue(PREFIX_DATE).isEmpty()) {
throw new ParseException(
String.format(MESSAGE_INVALID_COMMAND_FORMAT, ViewCommand.MESSAGE_USAGE)
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that he handled this under NuudleParser but I think it would be better to do the argument checking in this file instead?
Maybe can try doing the isEmpty check before tokenize?

Copy link
Collaborator

@xz0127 xz0127 left a comment

Choose a reason for hiding this comment

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

Other than the comments mentioned by Jinhao and Yang Yue, the rest LGTM! 👍

Copy link
Collaborator

@Avalionnet Avalionnet left a comment

Choose a reason for hiding this comment

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

LGTM other than the 2 comments mentioned 👍🏻

Comment on lines +28 to +31
if (argMultimap.getValue(PREFIX_DATE).isEmpty()) {
throw new ParseException(
String.format(MESSAGE_INVALID_COMMAND_FORMAT, ViewCommand.MESSAGE_USAGE)
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, as @YangYue128-helen mentioned, perhaps it'll be ok to leave out this check as we are able to support view commands without a DATE predicate as well?

Based on line 53 of ViewCommand.java, the current system seems to support the display of all future appointments if the predicate input is null (in the case that a date is not entered by the user) so I think you've got it covered @ZhangWanlin98 👍🏻

Comment on lines +28 to +31
if (argMultimap.getValue(PREFIX_DATE).isEmpty()) {
throw new ParseException(
String.format(MESSAGE_INVALID_COMMAND_FORMAT, ViewCommand.MESSAGE_USAGE)
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh wait after reviewing NuudleParser as mentioned by @JinHao-L , I think the check done here is actually valid as the ViewCommandParser is only instantiated if extra input by the user following the view command is detected. Hence, it actually indicates an invalid command format if PREFIX_DATE is empty as it would mean that something else is written.

Copy link
Collaborator

@JinHao-L JinHao-L left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@ZhangWanlin98 ZhangWanlin98 merged commit 06ac0ad into AY2021S1-CS2103T-T12-4:master Oct 13, 2020
@ZhangWanlin98 ZhangWanlin98 deleted the view_appointment branch October 13, 2020 06:18
@JinHao-L JinHao-L linked an issue Oct 13, 2020 that may be closed by this pull request
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.

Get overview of the day in clinic Bird’s eye view of all appointments
5 participants