Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
abdishakoor-dev
left a comment
There was a problem hiding this comment.
Your commit history shows the process this task is about: write a test, see it fail, then fix the code. You also kept the two starter tests exactly as they were, and the function gives the right answer for every valid time I tried.
Two things before I can mark this Complete:
- The midnight test uses an input that isn't in the range the README asks for. See my comment on
timeConverter.test.jsline 14. - One of the starter's bugs, and the edges where am changes to pm and back, have no test yet. See my comment on line 22.
The two comments on timeConverter.js are smaller and come out of the same idea: try deleting or changing a line, and see whether any test notices.
Add the Needs Review label again once you've pushed.
| }); | ||
|
|
||
| test("can correctly convert midnight ", function () { | ||
| assert.equal(formatAs12HourClock("24:00"), "12:00 am"); |
There was a problem hiding this comment.
On a 24-hour clock, does midnight show as 24:00 or 00:00? The clock in this task runs from 00:00 to 23:59, and the README only asks for valid inputs. Which input should this test use? Once you've changed it, do you still need lines 5 to 7 of timeConverter.js?
There was a problem hiding this comment.
Fixed now, and the 24:00 check has gone too. Good.
| }); | ||
|
|
||
| test("can correctly convert time with minutes ", function () { | ||
| assert.equal(formatAs12HourClock("12:45"), "12:45 pm"); |
There was a problem hiding this comment.
The starter dropped the minutes for afternoon times: "23:59" gave "11:00 pm". Try changing line 18 of timeConverter.js to return :00 instead of the minutes, and run your tests. Does any of them fail?
Bugs often hide at the exact point where something changes. What is the last minute before am turns into pm, and the last minute of the whole day? Each of those is worth its own test.
There was a problem hiding this comment.
Your 13:01 test catches this now for 1pm to 9pm. The last gap is after 10pm, see my new comment on line 15.
| if (time === "24:00") { | ||
| return `12:00 am`; | ||
| } | ||
| if (time === "12:00") { |
There was a problem hiding this comment.
If you delete lines 8 to 10, which if would "12:00" reach next? Would it still give "12:00 pm"?
There was a problem hiding this comment.
That's the one, deleted and the test still passes.
| if (time === "12:00") { | ||
| return `12:00 pm`; | ||
| } | ||
| if (stringHours == "00") { |
There was a problem hiding this comment.
Optional, not needed for Complete: line 14 checks hours with ===. Could line 11 check hours in the same way? If it did, would you still need stringHours on line 2?
There was a problem hiding this comment.
Good question! My initial thought was to compare string to a string thinking if I compare the number to a string it will give me false. Now that I read about it, I can compare the hour with == to the "00" and get the same answer. because in Java Script if you compare numeric with string, and if the sting can be changed to numeric value, the output can be true.
So I will make amends to the code based on this finding.
There was a problem hiding this comment.
Good reading, and removing stringHours made the function simpler. Nothing more needed here.
…n the program test success.
… codes .test was success.
…o testes failed made amend based on that
abdishakoor-dev
left a comment
There was a problem hiding this comment.
Good progress. Midnight and midday are both sorted, and the extra lines they needed are gone. Choosing 01:01 pm so afternoons match 08:00 am keeps the output consistent, nice.
You're very close. Two small things and this is Complete:
-
Add two more tests, written the same way as your others:
"11:59"should give"11:59 am"(the last minute before am turns into pm)"23:59"should give"11:59 pm"(the last minute of the day)
My comment on
timeConverter.jsline 15 explains why the second one matters. -
Both files need formatting again. Open
timeConverter.js, right click in the editor, choose Format Document, and save. Do the same fortimeConverter.test.js. Then commit and push.
Once both are pushed, add the Needs Review label again and I'll mark it Complete.
| return `0${hours - 12}:${mints} pm`; | ||
| } | ||
| if(hours>=22){ | ||
| return `${hours-12}:${mints} pm`; |
There was a problem hiding this comment.
In commit b1b6c72 you changed the minutes to 00, saw no test fail, and kept the line. That was the right experiment. When no test fails, it means a test is missing, not that the line is fine. A test that should have failed shows you where your tests have a gap.
The same gap is on this line now. If it returned :00 instead of the minutes, every test would still pass, because "23:00" is the only time you test after 10pm and its minutes are already 00. A "23:59" test closes that gap.
Learners, PR Template
Self checklist
Task code
CYF-1197
Changelist
wrote a test for clock edge cases.
Questions