Skip to content

Sheffield | 26-ITP-Jan | Mahmoud Shaabo | Sprint 3 | Alarm Clock App#1155

Open
mahmoudshaabo1984 wants to merge 2 commits intoCodeYourFuture:mainfrom
mahmoudshaabo1984:alarmclock-app
Open

Sheffield | 26-ITP-Jan | Mahmoud Shaabo | Sprint 3 | Alarm Clock App#1155
mahmoudshaabo1984 wants to merge 2 commits intoCodeYourFuture:mainfrom
mahmoudshaabo1984:alarmclock-app

Conversation

@mahmoudshaabo1984
Copy link
Copy Markdown

Hello Reviewers / CodeYourFuture Team,

I have completed the Alarm Clock App project for Sprint 3.

Acceptance Criteria Met:

  • Updated the title in index.html to "Alarm clock app".
  • Implemented setAlarm() in alarmclock.js to read the input and display the time in mm:ss format.
  • Used setInterval() to decrement the timer every 1 second.
  • When the timer reaches 00:00, the interval is cleared, the background changes to red, and the alarm audio plays.
  • The "Stop Alarm" button clears the interval, resets the background, and pauses the audio.
  • All Jest tests are passing locally.

Thank you for your review!

@mahmoudshaabo1984 mahmoudshaabo1984 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 31, 2026
@mahmoudshaabo1984
Copy link
Copy Markdown
Author

Hi @cifarquhar,

Here is my Pull Request for the Alarm Clock App. I successfully implemented the timer logic using setInterval and clearInterval, and handled the math to format the remaining seconds into a standard mm:ss display. All Jest tests passed successfully.

Looking forward to your feedback!

Copy link
Copy Markdown
Contributor

@cjyuan cjyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Can you make the "Stop" button to also stop the count down? You can add another "click" event listener to the button to avoid modifying the original code.

  • When input is 1, the alarm is played one second later -- an expected behavior.
    However, when input is 0, the alarm is also played one second later (instead of being played immediately). Can you make the app's behaviour more consistent?

const mins = String(Math.floor(totalSeconds / 60)).padStart(2, "0");
const secs = String(totalSeconds % 60).padStart(2, "0");
document.getElementById("timeRemaining").innerText =
"Time Remaining: " + mins + ":" + secs;
Copy link
Copy Markdown
Contributor

@cjyuan cjyuan Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of keeping several copies of the code for displaying time (lines 18-21, lines 44-47, lines 32-33),
it is better to define a function to update the time display and call the function to update the time display.

Advantages:

  • Keep the logic for updating time display in one place
  • Less code

document.body.style.backgroundColor = "";

// Read the input value and convert it to a plain integer (total seconds)
let totalSeconds = parseInt(document.getElementById("alarmSet").value);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some integer values or unusual input can still make your app behave abnormally. Can you figure out a way to prevent that from happening?

Comment on lines +5 to +10
// Clear any previously running countdown
clearInterval(interval);

// Reset background to default
document.body.style.backgroundColor = "";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. In addition to resetting the countdown and the background color, what else should also be reset? (Hint: a user may not click the "Stop" button first before starting a new count down.)

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Apr 5, 2026
@mahmoudshaabo1984
Copy link
Copy Markdown
Author

Hi CJ,

Thank you for the excellent feedback! I have just pushed an update to address all your points:

  1. Created a helper function to format and display the time, avoiding code duplication (DRY principle).
  2. Made sure the "Stop" button also clears the countdown interval.
  3. Updated the logic so if the input is 0, the alarm triggers immediately.
  4. Added validation to reject empty inputs or negative numbers.
  5. Ensured pauseAlarm() is called at the start of a new alarm to maintain a clean state.

All Jest tests are still passing successfully. Please let me know if everything looks good to go!

Best regards,
Mahmoud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Reviewed Volunteer to add when completing a review with trainee action still to take.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants