London | 26-ITP-May | Damilola Odumosu| Sprint 3 | Alarm Clock - #1360
London | 26-ITP-May | Damilola Odumosu| Sprint 3 | Alarm Clock#1360d-odumosu wants to merge 4 commits into
Conversation
d-odumosu
added
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
-
Code works fine if a user only clicks the "Set Alarm" button once. However, if the user enters a time and then clicks the "Set Alarm" button multiple times, the countdown clock will not display properly. Could you fix the issue?
-
Currently when starting a new countdown, the application does not always return to a clean initial state, which can lead to inconsistent behaviour between runs.
Note: a user may not click the "Stop" button first before starting a new count down.
| @@ -1,4 +1,28 @@ | |||
| function setAlarm() {} | |||
| const input = document.getElementById("alarmSet"); | |||
| const timeDisplay = document.querySelector("span"); | |||
There was a problem hiding this comment.
span is a fairly generic element, so using it as the selector for the time display can make the code less specific and more fragile. Could you find out why selecting a span for this purpose is not considered good practice, and update the selector accordingly?
There was a problem hiding this comment.
i have added and id and used getelemntbyid.
| let remainingMinutes = Math.floor(timeInput / 60); | ||
| let remainingSeconds = timeInput % 60; | ||
| remainingMinutes = remainingMinutes.toString().padStart(2, "0"); | ||
| remainingSeconds = remainingSeconds.toString().padStart(2, "0"); | ||
| timeDisplay.textContent = `${remainingMinutes}:${remainingSeconds}`; |
There was a problem hiding this comment.
Code on lines 15-19 is very similar to those on lines 6-11.
To adhere to the DRY principle, could you refactor the repeated code into a reusable function?
| const timeDisplay = document.querySelector("span"); | ||
|
|
||
| function setAlarm() { | ||
| let timeInput = Number(input.value); |
There was a problem hiding this comment.
Some input values could make your app behave abnormally. Could you add code to sanitise or reject them?
There was a problem hiding this comment.
I have added validation to the input
cjyuan
added
Reviewed
d-odumosu
added
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Changes look good, but there are a few minor improvement you could still make.
I will mark this PR as "Complete" first.
|
|
||
| function getTime() { | ||
| const time = Number(input.value); | ||
| if (!(Number.isFinite(time) && Number.isInteger(time) && time >= 1)) { |
There was a problem hiding this comment.
This check is good.
Please note that one of the conditions is redundant.
There was a problem hiding this comment.
I think this Number.isFinite(time) is redundant, will make the change
| function resetAlarm() { | ||
| clearInterval(intervalId); | ||
| intervalId = null; | ||
| timeRemaining.textContent = `Time Remaining: 00:00`; |
There was a problem hiding this comment.
Why not call the function you have implemented for displaying time?
| function updateTime(updatedTime) { | ||
| const time = updatedTime; |
There was a problem hiding this comment.
Why not just name the parameter time?
cjyuan
added
Complete
2 participants
Learners, PR Template
Self checklist