London | 25-ITP-Sept | Samuel Tarawally | Sprint 3 | Alarm Clock #929
Conversation
|
Your PR's title isn't in the expected format. Please check the expected title format, and update yours to match. Reason: Wrong number of parts separated by |s If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above. |
Tarawally
changed the title
Tarawally
added
the
Needs Review
| let formattedMinutes = `${minutes}`; | ||
| if (minutes < 10) { | ||
| formattedMinutes = `0${minutes}`; | ||
| } | ||
|
|
||
| let formattedSeconds = `${seconds}`; | ||
| if (seconds < 10) { | ||
| formattedSeconds = `0${seconds}`; | ||
| } | ||
|
|
||
| return `Time Remaining: ${formattedMinutes}:${formattedSeconds}`; |
There was a problem hiding this comment.
Code on lines 13-23 could be simplified by using String.prototype.padStart().
| * Updates the display and checks if the alarm should sound. | ||
| */ | ||
| function updateTime() { | ||
| const titleElement = document.getElementById("timeRemaining"); |
There was a problem hiding this comment.
We could perform the operation on line 30 only once and reuse the retrieved DOM element repeatedly (to improve performance).
| * Initialises the alarm countdown. | ||
| */ | ||
| function setAlarm() { | ||
| if (alarmTimerIdentifier) { |
There was a problem hiding this comment.
Once set, alarmTimerIdentifier is never reset. So this condition will only be false the first time setAlarm() is called. Well, do we need to check or reset this variable?
| const titleElement = document.getElementById("timeRemaining"); | ||
| titleElement.innerText = formatTime(timeRemainingInSeconds); |
There was a problem hiding this comment.
These two statements and the statements on lines 30, 33 are identical. This is usually a hint that we could factor out the code into a function, and then just call the function to perform the corresponding task.
| alarmTimerIdentifier = setInterval(() => { | ||
| updateTime(); | ||
| }, ONE_SECOND_IN_MILLISECONDS); | ||
| } |
There was a problem hiding this comment.
The alarm will sound one second afterward when the input is either 0 or 1 second -- it is a bit inconsistent. Can you improve the consistency?
cjyuan
added
Reviewed
2 participants
Learners, PR Template
Self checklist
Changelist