-
Notifications
You must be signed in to change notification settings - Fork 5
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
Modify dateOfLastVisit to use LocalDate instead of String #171
Modify dateOfLastVisit to use LocalDate instead of String #171
Conversation
Codecov ReportAttention: Patch coverage is
|
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.
Overall good changes, but I have some concerns regarding the testing. Please see comments.
src/test/java/seedu/address/logic/commands/SortCommandTest.java
Outdated
Show resolved
Hide resolved
src/test/java/seedu/address/logic/commands/SortCommandTest.java
Outdated
Show resolved
Hide resolved
// Days 01-31 for other months | ||
// ------------------- | ||
public static final String MESSAGE_CONSTRAINTS = "Date of last visit should be in dd-MM-yyyy format.\n" | ||
+ "Ensure that the month is 01-12 and the date is not later than today. "; |
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.
I don't quite understand the ensure month is 01-12 comment. Do you mean ensure that month is after day?
Edit: Oh I think you mean ensure the month is typed with 2 digits, but I think you can take this as a given since we requested dd-MM-yyyy format. You can say "Date of Last Visit should be in exactly dd-MM-yyyy format. Also it is good to add an example in the error message.
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.
yup that was the intention and i will update the error message as suggested
@@ -12,7 +13,7 @@ public class PersonComparator { | |||
public static final String NAME = "name"; | |||
public static final String DATE_OF_LAST_VISIT = "date of last visit"; | |||
public static final String EARLIEST_VALID_DATE = "01-01-0001"; | |||
public static final String LATEST_VALID_DATE = "31-12-9999"; | |||
public static final String LATEST_VALID_DATE = LocalDate.now().format(DateOfLastVisit.DATE_TIME_FORMATTER); |
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.
It may be better to not change this. The LATEST_VALID_DATE is meant to distinguish entries with no date of last visit from those that do have. I believe in this case if you set the date of last visit to be today for one of the entries and then sort the entry with dolv as today will be mixed with those without date of last visit. Hence, it is better to keep the value as is or to set it as today + 1.
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.
DateOfLastVisit has been updated to accept a date no later than today, which means that a date of "31-12-9999" would not be possible. I have updated the comparators for date of last visit to not use EARLIEST_VALID_DATE and LATEST_VALID_DATE. Although this makes the code slightly longer and less clean, the functionality works as expected.
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.
LGTM
1d68778
into
AY2425S1-CS2103-F10-2:master
This PR completed the following things
closes #141
closes #166