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 6 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.
| 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.
cjyuan
added
Reviewed
…ect with css animation, fixed missing functions and broken format logic
Yonatanteklemariam
added
Needs Review
| const mins = Number(minsStr); | ||
| const secs = Number(secsStr); | ||
|
|
||
| if ( | ||
| Number.isNaN(mins) || | ||
| Number.isNaN(secs) || |
There was a problem hiding this comment.
What type of numbers should they be? What if the user enter 1.1:2.2 or 1.2345?
|
|
||
| document.getElementById("stop").addEventListener("click", () => { | ||
| pauseAlarm(); | ||
| stopFlashing(); // stop flashing if running |
There was a problem hiding this comment.
An alternative would be to add another event listener that performs only `stop flashing".
cjyuan
added
Complete
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