London | 26-ITP-Sep | Alan Mak | Sprint 3 | Module-JavaScript-Fundamentals Coursework - #1619
AlanGit-debug2604 wants to merge 26 commits into
Conversation
… explained, and code corrected.
…nd log result to one decimal place
…to convert strings to UPPER_SNAKE_CASE format
…nvert whole pence string to formatted pounds
…Pound function for clarity; time-format.js updated comments with answers to questions
…nputs over 24 and update test cases for clarity, also handle minutes input other than :00
This comment has been minimized.
This comment has been minimized.
✅ Deploy Preview for cyf-onboarding-module ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
AlanGit-debug2604
added
📅 Sprint 3
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
removed
the
Needs Review
This comment has been minimized.
This comment has been minimized.
AlanGit-debug2604
added
the
Needs Review
AlanGit-debug2604
changed the title
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
In 5-stretch-extend, Further edge cases where input hours >36. Because of this, I m wondering if it is more preferable to leverage on something similar from time-format.js, and build on const hours_in_day = 24, then using remainder..etc, rather than using if.
The spec does not mention what to do for hours > 24. So you can decide what to do when that happened.
Without seeing the actual code, I could not compare the approaches you were describing.
| if (hours > 24) { | ||
| return `${hours - 24}:${minutes} am +1`; |
There was a problem hiding this comment.
The +1 is shorthand for "plus one day" meaning the time falls on the next day
|
|
||
| console.log(formatAs12HourClock("08:01")); | ||
| console.log(formatAs12HourClock("23:55")); | ||
| console.log(formatAs12HourClock("25:01")); |
There was a problem hiding this comment.
What output do you expect from console.log(formatAs12HourClock("00:01"))
There was a problem hiding this comment.
Good catch thanks! Fixed to ensure leading zeros for single-digit hours, and to ensure correct am/pm representation
cjyuan
added
Reviewed
…ed if (hours >= 24) (previous >24)
…nd 22, ensuring proper AM/PM representation
AlanGit-debug2604
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Changes look good.
Thanks for responding to all comments.
Since "stretch" exercise is optional, I will mark this PR as "Complete" first. You could continue improving the function in the stretch exercise if time permits.
| const currentOutput4 = formatAs12HourClock("00:01"); | ||
| const targetOutput4 = "00:01 am"; |
There was a problem hiding this comment.
In 12-hour clock, 00:01 should be converted to "12:01 am".
| } else if (hours > 22) { | ||
| return `${hours - 12}:${minutes} pm`; | ||
| } else if (hours > 12) { | ||
| return `0${hours - 12}:${minutes} pm`; |
There was a problem hiding this comment.
This is not wrong, but the code could be simplified by using the .padStart() method of String.
Another approach you could consider is:
- Introduce variables to store the values needed to produce the resulting string
- Compute the value of these variables
- Produce the the resulting string from these variables (only one such statement is needed)
cjyuan
added
Complete
…tion for edge cases (e,g, input HH = 00)
2 participants
Alan Mak, Sprint-3 PR
Self checklist
CYF-1053
CYF-1053
Changelist
Coursework for Sprint-3 covering Functions, Scoping, passing arguments value to parameter.
Questions
In 5-stretch-extend, Further edge cases where input hours >36. Because of this, I m wondering if it is more preferable to leverage on something similar from time-format.js, and build on
consthours_in_day = 24, then using remainder..etc, rather than usingif.