Skip to content

Glasgow| 2026-ITP-Jan | Tuan Nguyen| Sprint 3 | Alarm clock#1209

Open
Jacknguyen4438 wants to merge 7 commits intoCodeYourFuture:mainfrom
Jacknguyen4438:Sprint-3-alarmClock
Open

Glasgow| 2026-ITP-Jan | Tuan Nguyen| Sprint 3 | Alarm clock#1209
Jacknguyen4438 wants to merge 7 commits intoCodeYourFuture:mainfrom
Jacknguyen4438:Sprint-3-alarmClock

Conversation

@Jacknguyen4438
Copy link
Copy Markdown

Learners, PR Template

Self checklist

  • I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
  • My changes meet the requirements of the task
  • I have tested my changes
  • My changes follow the style guide

Changelist

Hello I have the function alarm clock and is working correctly. If additional change need to be done I will fix it right away.

Questions

If this PR have been done mark as completed, can I come back and do the additional task later?

@Jacknguyen4438 Jacknguyen4438 added 📅 Sprint 3 Assigned during Sprint 3 of this module Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Module-Data-Groups The name of the module. labels Apr 21, 2026
Comment thread Sprint-3/alarmclock/alarmclock.js Outdated

function setAlarm() {
const input = document.getElementById("alarmSet");
let time = Number(input.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.

What type and range of number should time be? Invalid user input could cause the app to behave abnormally.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thank you for the feed back I have add validation cod to help with this here is my validation code below:

if (isNaN(time) || time < 0) {
updateHeading(0);
return;
}

Copy link
Copy Markdown
Contributor

@cjyuan cjyuan Apr 22, 2026

Choose a reason for hiding this comment

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

What do you expect the app to behave when the user enters 3.14 as input?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thank you for the feed back here is my explain when decimal is pass down:

If the user enters 3.14, Number() converts it to a floating‑point number.
The countdown will decrease by 1 each second (3.14 → 2.14 → 1.14 → 0.14 → alarm), but the display will show whole seconds because of the formatting.

const minutes = Math.floor(time / 60);
const seconds = time % 60;

This behaviour is technically valid but not intuitive for users, since countdown timers usually expect whole seconds.

A more predictable behaviour would be to either:

reject decimal input as invalid, or

convert it to an integer using Math.floor() or Math.round().

This would make the app’s behaviour clearer and more consistent.

Thank you for the reading and I will some change base on this explanation to make sure the function work properly.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In general if you're just ignoring user input (as you do if someone puts in a float), it's good to show an error message telling them what's wrong, rather than just ignore it.

Also, if you already had an alarm counting down, then submitted 1.2, it will set to 00:00 but then shortly after start counting down again.

Can you fix up both of these things?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hello @illicitonion thank you for your feed back I have understand what your like to ask me to do so here is my validation code that will show on the HTML page:

if (isNaN(time) || time < 0 || !Number.isInteger(time)) {
showError("Please enter a whole number of seconds.");
return;
}

Comment thread Sprint-3/alarmclock/alarmclock.js Outdated
Comment thread Sprint-3/alarmclock/alarmclock.js Outdated
@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 21, 2026
@Jacknguyen4438
Copy link
Copy Markdown
Author

Jacknguyen4438 commented Apr 21, 2026

Hello @cjyuan, thank you for your review my code is ready to be review again if you need me to max more change please let know.

@Jacknguyen4438 Jacknguyen4438 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 21, 2026
@cjyuan cjyuan removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 22, 2026
@Jacknguyen4438
Copy link
Copy Markdown
Author

Hello @cjyuan and thank you for the feed back I make small change so that the function can correctly handle float number. Also I have answer some of the question you would like me to answer. I hope to hear from you soon thank you

@Jacknguyen4438 Jacknguyen4438 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 22, 2026
@illicitonion illicitonion removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 23, 2026
@Jacknguyen4438 Jacknguyen4438 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 23, 2026
@Jacknguyen4438
Copy link
Copy Markdown
Author

@illicitonion Hello and thank you for your time review my PR, I have make change to fit with you feed back if I need make change for the error message please let me know

@cjyuan
Copy link
Copy Markdown
Contributor

cjyuan commented Apr 23, 2026

Changes look good.

@cjyuan cjyuan added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Reviewed Volunteer to add when completing a review with trainee action still to take. labels Apr 23, 2026
Comment thread Sprint-3/alarmclock/alarmclock.js Outdated
Comment on lines +11 to +12
const heading = document.getElementById("timeRemaining");
heading.innerText = message;
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.

Would be better to introducing a separate element to showing message.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@cjyuan thank you for you reply I have making change that now will display the error without interfering with the heading element in HTML page. Here is my solution:

function showError(message) {
const heading = document.getElementById("errorMessage");
heading.innerText = message;
}

function setAlarm() {
const input = document.getElementById("alarmSet");
let time = Number(input.value);

resetAlarm();

if (isNaN(time) || time < 0 || !Number.isInteger(time)) {
showError("Please enter a whole number of seconds.");
return;
}

document.getElementById("errorMessage").innerText = "";

for better checking I have make an commit with full code so you can check for better view of the whole code. Thank you.

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

Labels

Complete Volunteer to add when work is complete and all review comments have been addressed. Module-Data-Groups The name of the module. 📅 Sprint 3 Assigned during Sprint 3 of this module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants