Repository navigation
London | 26-ITP-Sep | Chandramani Gaire | Sprint 1 | Exhaustively test and fix formatAs12HourClock - #1649
Conversation
This comment has been minimized.
This comment has been minimized.
gaireprakash20-ops
left a comment
gaireprakash20-ops
left a comment
There was a problem hiding this comment.
i did as per instruction
gaireprakash20-ops
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 from 00:00 to 23:59, and your tests catch all three of the starter's bugs. Testing 23:05 to check single-digit minutes after noon was a good catch.
Three things before I can mark this Complete:
- A few exact boundaries have no test yet. See my comment on
timeConverter.test.jsline 13. timeConverter.jslines 1 to 16 still hold the old starter code in comments. See my comment there.timeConverter.test.jslines 32 to 39 are commented-out tests for inputs the README says you don't need to handle. See my comment there.
Add the Needs Review label again once you've pushed.
| assert.equal(formatAs12HourClock("08:00"), "08:00 am"); | ||
| }); | ||
|
|
||
| test("can correctly convert midnight with double digit minutes", function () { |
There was a problem hiding this comment.
This tests a time inside the midnight hour, which is good. What about midnight itself, "00:00"? The same goes for noon, "12:00", and the very last minute of the day, "23:59". Bugs often hide at the exact point where something changes, so each of those is worth its own test.
There was a problem hiding this comment.
All three are in now. Good.
| @@ -1,11 +1,34 @@ | |||
| function formatAs12HourClock(time) { | |||
| // This code only work when there is only hours not whenn there is minutes. | |||
There was a problem hiding this comment.
Git already keeps the starter version in your commit history, so this commented-out copy isn't needed. Please delete lines 1 to 16, and the comment on line 17 too if it no longer makes sense without them. The file should only hold the code that runs.
There was a problem hiding this comment.
Gone, and the file is much easier to read.
| test("can correctly convert one hour after noon", function () { | ||
| assert.equal(formatAs12HourClock("13:00"), "01:00 pm"); | ||
| }); | ||
| // It's a failing case also we need to complete it |
There was a problem hiding this comment.
The README says "You don't need to worry about invalid inputs (e.g. "25:00")". So these two tests aren't part of the task, and commented-out code should be removed rather than left in. Please delete lines 32 to 39.
There was a problem hiding this comment.
remove the unnecessary code
abdishakoor-dev
added
Reviewed
gaireprakash20-ops
left a comment
gaireprakash20-ops
left a comment
There was a problem hiding this comment.
i changed as per the review
gaireprakash20-ops
added
Needs Review
abdishakoor-dev
left a comment
abdishakoor-dev
left a comment
There was a problem hiding this comment.
All sorted, marking it Complete. Well done.
abdishakoor-dev
added
Complete
2 participants
Learners, PR Template
Self checklist
Task code
CYF-1197
Changelist
I have completed the exercise as per the instructions provided.