Open LTK-1606 opened 1 week ago
Thank you for your feedback. We understand that implementing a logic checker should be done in the project lifecycle.
However, as we tried to implement this, we realized that it would be more complicated as our add
and update
command do not have restrictions on the order of the flags, -to
flag can come before -from
flag and the command will still be accepted. This design choice is to give users more freedom in typing the fields, so there is no need to repeatedly backspace for missing fields. This also made the date logic checker more complicated to implement, to the point that we concluded that it will not be ready by the project deadline.
The lack of a date logic check will not detract heavily from our use case as users can easily update the dates if they spot an issue with the dates.
Thus, we decided to delay the implementation of the feature.
Team chose [response.NotInScope
]
Reason for disagreement:
I looked through the code and it does not seem like the order of the flags will make it more complicated at all. Even if the user writes the from and to date flags in a different order, the respective dates are still saved under the correct startDate and endDate variables in your AddCommand class since it is using a switch case to look for the actual flag name (similar for UpdateCommand class).
Minimal effort is needed to convert those dates saved as a string into a date-time format (which you already did since the dates are already saved in YearMonth format in the end), to compare them with one another and then to throw an error for only 1 specific scenario (when endDate is before startDate). It would require another 5-7 lines of code max including throwing the error. There are also in-built methods for the YearMonth class like isBefore and isAfter.
It could be as simple as first converting both startDate and endDate to a YearMonth format then
if(endDate.isBefore(startDate)) {
throw INVALID_DATE_EXCEPTION //however you do error handling
}
before creating a new Internship(...)
The inclusion of such a simple logic check will take very little effort and definitely makes the feature better from the user point of view, hence it should not be classified as not in scope.
Expected: Shows an error that the ending date is in the past hence this is an invalid date range
Actual: Date range is accepted
I am aware it is stated in the UG that this feature is coming soon, but it is a pretty standard and easy logic check to include, especially considering that is pretty easy to make a typo or to mix up the mm/yy to yy/mm