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

Fix invalid day bug #256

Merged

Conversation

AlyssaPng
Copy link
Collaborator

fix #241

@AlyssaPng AlyssaPng added the bug Something isn't working label Nov 10, 2023
@AlyssaPng AlyssaPng added this to the v1.4 milestone Nov 10, 2023
Copy link
Collaborator

@sopa301 sopa301 left a comment

Choose a reason for hiding this comment

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

Thanks for the good effort. I think the SLAP is done quite well, though the naming could be a bit more intuitive. Some other things I noticed:

  • Javadoc for parseAppointment (aptName and aptDate are outdated).
  • (Nitpick) The validation for the Date and date part of DateTime is duplicated. Perhaps we could extract it into a method?

src/main/java/seedu/address/logic/parser/ParserUtil.java Outdated Show resolved Hide resolved
src/main/java/seedu/address/logic/parser/ParserUtil.java Outdated Show resolved Hide resolved
src/main/java/seedu/address/logic/parser/ParserUtil.java Outdated Show resolved Hide resolved
@sopa301
Copy link
Collaborator

sopa301 commented Nov 10, 2023

@Kb-Tay I'd like your input on these too, since you're the original author of this code.
@jylow Maybe you could do that bug finding magic with this as well?

Copy link
Collaborator

@Kb-Tay Kb-Tay left a comment

Choose a reason for hiding this comment

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

Overall, the code implemention is good in readability and has appropiate SLAP. However, can make some edits to prevent repetition of code. Can also consider including more test cases to ensure its comprehensiveness

src/main/java/seedu/address/logic/parser/ParserUtil.java Outdated Show resolved Hide resolved
src/main/java/seedu/address/logic/parser/ParserUtil.java Outdated Show resolved Hide resolved
Copy link
Collaborator

@sopa301 sopa301 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

codecov bot commented Nov 11, 2023

Codecov Report

Merging #256 (dcd9d97) into master (ed176dc) will decrease coverage by 0.04%.
Report is 6 commits behind head on master.
The diff coverage is 86.36%.

@@             Coverage Diff              @@
##             master     #256      +/-   ##
============================================
- Coverage     75.53%   75.49%   -0.04%     
- Complexity      670      676       +6     
============================================
  Files           103      103              
  Lines          2166     2175       +9     
  Branches        220      223       +3     
============================================
+ Hits           1636     1642       +6     
- Misses          467      469       +2     
- Partials         63       64       +1     
Files Coverage Δ
.../seedu/address/logic/commands/ScheduleCommand.java 61.29% <ø> (ø)
...a/seedu/address/model/appointment/Appointment.java 85.00% <100.00%> (+0.38%) ⬆️
...in/java/seedu/address/logic/parser/ParserUtil.java 94.00% <82.35%> (-2.74%) ⬇️

... and 1 file with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@AlyssaPng AlyssaPng merged commit ddc9c57 into AY2324S1-CS2103T-F12-1:master Nov 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding invalid date causes a bug
3 participants