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
Sprint-3/alarmclock/alarmclock.js
Outdated
| 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().
Sprint-3/alarmclock/alarmclock.js
Outdated
| * 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?
There was a problem hiding this comment.
The variable needs to be reset otherwise the condition will always be true after the first alarm.
Sprint-3/alarmclock/alarmclock.js
Outdated
| 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?
There was a problem hiding this comment.
I'll add a conditional statement to handle this case.
cjyuan
added
Reviewed
|
I have refactored the code to address the points:
|
Tarawally
added
Needs Review
| */ | ||
| function setAlarm() { | ||
| if (alarmTimerIdentifier) { | ||
| clearInterval(alarmTimerIdentifier); |
There was a problem hiding this comment.
If this function returns on line 56, 60, or 75, alarmTimerIdentifier will remain unchanged even though the corresponding interval is cleared here.
cjyuan
added
Complete
2 participants
Learners, PR Template
Self checklist
Changelist