London | 26-ITP-May | Yonatan Teklemariam | Sprint 3 | Alarm Clock - #1403
London | 26-ITP-May | Yonatan Teklemariam | Sprint 3 | Alarm Clock#1403Yonatanteklemariam wants to merge 5 commits into
Conversation
Yonatanteklemariam
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
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.
| let timeLeft = totalSeconds; | ||
|
|
||
| // If a previous timer was running, stop it | ||
| if (window.countdownTimer) { |
There was a problem hiding this comment.
Why store countdownTimer in the window object, but keep flashInterval in the global scope of this file?
| document.getElementById("timeRemaining").innerText = | ||
| `Time Remaining: ${formatTime(timeLeft)}`; |
There was a problem hiding this comment.
Could consider implement a function to display the time instead of repeating the logic several times.
| // When the timer reaches zero | ||
| if (timeLeft <= 0) { | ||
| clearInterval(window.countdownTimer); // stop the countdown | ||
| clearInterval(flashInterval); // stop the flashing if it was running |
There was a problem hiding this comment.
Why would the background be flashing when a count down was active? (Should the app prevent it from happening?)
| const input = document.getElementById("alarmSet").value; | ||
|
|
||
| // If nothing was typed, exit the function | ||
| if (!input) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Is there any other value the app should also reject (to prevent it from behaving abnormally)?
| let timeLeft = totalSeconds; | ||
|
|
||
| // If a previous timer was running, stop it | ||
| if (window.countdownTimer) { | ||
| clearInterval(window.countdownTimer); | ||
| } | ||
|
|
||
| // A simple helper to turn seconds into MM:SS format | ||
| function formatTime(seconds) { |
There was a problem hiding this comment.
Interleaving variable declarations, function definitions, and executable code makes the code harder to read and maintain. A common practice is to organise the code in the following order:
// Variable declarations
// Function definitions
// Code to be executed
| if (input.includes(":")) { | ||
| // Split into minutes and seconds | ||
| const parts = input.split(":"); | ||
| const minutes = Number(parts[0]); | ||
| const seconds = Number(parts[1]); | ||
|
|
||
| // Convert everything into total seconds | ||
| totalSeconds = minutes * 60 + seconds; | ||
| } else { | ||
| // Otherwise treat the input as normal seconds | ||
| totalSeconds = Number(input); | ||
| } |
There was a problem hiding this comment.
As the logic for converting an input string to seconds becomes more complicated, it is better to encapsulate the conversion logic in a function. Doing so makes the code easier to read and understand.
| function startFlashing() { | ||
| let isRed = false; // keeps track of which color we should show | ||
|
|
||
| flashInterval = setInterval(function () { | ||
| if (isRed) { | ||
| document.body.style.backgroundColor = "white"; | ||
| } else { | ||
| document.body.style.backgroundColor = "red"; | ||
| } | ||
|
|
||
| // Switch the color for next time | ||
| isRed = !isRed; | ||
| }, 500); // run every half second | ||
| } | ||
|
|
||
| // This function stops the flashing and resets the background | ||
| function stopFlashing() { | ||
| clearInterval(flashInterval); // stop the flashing interval | ||
| document.body.style.backgroundColor = "white"; // reset background | ||
| } |
There was a problem hiding this comment.
If time permits, you could consider implement the flashing background using CSS, and then implement only one function to enable/disable flashing as:
function enabletFlashing(enable) { // true or false
if (enable) {
// Add the CSS class to the body element
}
else {
// Remove the CSS class from the body element
}
}
cjyuan
added
Reviewed
2 participants
Learners, PR Template
Self checklist
Changelist
Implemented the alarm clock function based on the specified instruction and added some flashing effect to it
Questions