Repository navigation
London | 26-ITP-Sep| Mars Adesina | Sprint 1 | formatAs12HourClock - #1640
marscancode wants to merge 6 commits into
Conversation
marscancode
added
📅 Sprint 1
abdishakoor-dev
left a comment
abdishakoor-dev
left a comment
There was a problem hiding this comment.
Your function gives the right answer for every valid time I tried, including midnight, noon and the minute either side of each. You also kept the two starter tests exactly as they were, and the code passes Prettier.
Two things before I can mark this Complete:
- Nothing tests the 12 o'clock hour yet. See my comment on
timeConverter.jsline 9. - Some of the edges where am changes to pm, and back, have no test. See my comment on
timeConverter.test.jsline 17.
The other two comments are smaller: a test name that doesn't match its input, and an optional question about formatting.
Add the Needs Review label again once you've pushed.
| }); | ||
|
|
||
| test("can correctly convert hours in the afternoon", function () { | ||
| assert.equal(formatAs12HourClock("13:00"), "1:00 pm"); |
There was a problem hiding this comment.
Optional, not needed for Complete: morning times keep their zero ("08:00 am"), but afternoon times drop it ("1:00 pm"). Which style do you think the function should use? Whichever you pick, it's worth being the same for both.
abdishakoor-dev
added
Reviewed
marscancode
added
Needs Review
abdishakoor-dev
left a comment
abdishakoor-dev
left a comment
There was a problem hiding this comment.
The new 12:00 test is the one that was missing: if I delete lines 9 and 10 of timeConverter.js, it fails straight away. With 11:59 and 23:59 covered too, that's everything. Marking this Complete, well done.
abdishakoor-dev
added
Complete
2 participants
Learners, PR Template
Self checklist
CYF-1197
Changelist
I added several edge case tests to help me locate the bugs in the original code and after finding out that it failed to convert midnight and minutes after 12, I updated the function so it now passes those same edge cases tests.