Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
illicitonion
left a comment
There was a problem hiding this comment.
This works and generally looks good, but I left a few comments to think about :)
| test("can format afternoon time with minutes other than 00", () => | ||
| assert.equal(formatAs12HourClock("15:45"), "03:45 pm")); | ||
|
|
||
| test("can format morning time with complex minutes", () => | ||
| assert.equal(formatAs12HourClock("08:25"), "08:25 am")); | ||
|
|
||
| test("can format early noon complex minutes", () => | ||
| assert.equal(formatAs12HourClock("12:17"), "12:17 pm")); | ||
|
|
||
| test("can format between midnight and 1 am", () => |
There was a problem hiding this comment.
These are really good tests but you're using inconsistent terminology here - sometimes you're saying "minutes other than 00" and other times "complex minutes". By using different terms it makes me as a reader wonder whether they have different meanings. If you mean the same thing, I'd recommend using the same term.
| test("correctly convert time after 12:00", function(){ | ||
| assert.equal(formatAs12HourClock("23:00"), "11:00 pm"); | ||
| }); | ||
| test("correctly convert time after 12:00", () => |
There was a problem hiding this comment.
This looks like a pretty thorough set of tests - well done!
|
|
||
| const hours = Number(time.slice(0, 2)); | ||
|
|
||
| if (hours > 12) { |
There was a problem hiding this comment.
I notice that in several of your branches you're doing the same thing - writing time.slice(-2) - if you had to change that for some reason, you'd need to change both copies. Can you think how to avoid this duplication?
| } else if (hours === 12) { | ||
| return `${time} pm`; | ||
| } else if (hours === 0) { | ||
| return `12:${time.slice(-2)} am`; |
There was a problem hiding this comment.
I've noticed that all of these branches have something in common - at its core, all of them are calculating an "hours", calculating a "minutes", and appending an "am" or "pm"
Often it can be useful to make clear in code what things are the same and what things are different. Can you think how you may structure this code so that you always just return ${hours}:${minutes} am/pm, but make clear with your if statement how you're differently computing those things?
Learners, PR Template
Self checklist
Task code
CYF-1197
Changelist
I wrote tests for the function for as many edge cases I could think of, and corrected the function to pass the tests when the tests failed.