Manchester| 25-ITP-Sep| Fithi Teklom| Sprint 3 | Alarm Clock #908
Conversation
Fithi-Teklom
added
the
Needs Review
JaypeeLan
added
Review in progress
Sprint-3/alarmclock/alarmclock.js
Outdated
| const input = document.getElementById("alarmSet").value; | ||
| const parsed = parseInt(input, 10); | ||
|
|
||
| if (isNaN(parsed) || parsed < 0) return; |
There was a problem hiding this comment.
You should check for an empty value first. Also alert an error if any.
There was a problem hiding this comment.
hello, can you please review my PR again it has been a while. step 3 has not approved because this PR is still in needs review
JaypeeLan
added
Reviewed
Fithi-Teklom
added
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Indentation is a bit off. Can you improve the indentation?
Sprint-3/alarmclock/alarmclock.js
Outdated
| timer = setInterval(() => { | ||
| if (timeLeft > 0) { | ||
| timeLeft--; | ||
| updateDisplay(timeLeft); | ||
| } | ||
|
|
||
| if (timeLeft === 0) { | ||
| clearInterval(timer); | ||
| startAlarm(); | ||
| } | ||
| }, 1000); | ||
| } |
There was a problem hiding this comment.
The alarm will start 1 second after the alarm is set when the input is either 0 or 1.
That is, when the input is zero, the alarm will only start 1 second afterward (and not immediately).
Can you improve the consistency?
cjyuan
added
Reviewed
Fithi-Teklom
added
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Can you improve the indentation of the code in this file?
VSCode has a built-in "Format document" feature that can be used to automatically indent JS code.
- Need to install "prettier" VSCode extension first.
- To auto-indent JS code, right-click in the editor and select "Format document" to format the code in the editor.
Sprint-3/alarmclock/alarmclock.js
Outdated
| clearInterval(timer); | ||
| clearInterval(flashing); // stop flashing if it was active | ||
| flashing = null; | ||
| document.body.style.backgroundColor = ""; | ||
| pauseAlarm(); // stop any playing audio | ||
| audio.currentTime = 0; |
There was a problem hiding this comment.
Could consider placing this "reset" code in a function and call the function here.
Sprint-3/alarmclock/alarmclock.js
Outdated
| tick(); | ||
| timer = setInterval(tick, 1000); |
There was a problem hiding this comment.
Considering the following two cases:
Case 1: When the input is 0 (i.e., timeLeft is 0)
Line 69 will also be executed, resulting the code on line 62-63 being executed twice.
Case 2: When the input is 1 (i.e., timeLeft is 1)
Line 68 is executed, causing the display to show "00:00" immediately (instead of showing "00:01").
But startAlarm() is only executed one second afterward.
(Ideally, the alarm could sound as soon as the display show "00:00")
Suggestion: Since 0 is such a special case, you can consider doing something like
if (timeLeft === 0) {
}
else { // Other other cases
}
3 participants
Self checklist