NW | ITP-May-25 | Geraldine Edwards | Module-Data-Groups | Sprint-3 | Alarm Clock App #585
Conversation
Geraldine-Edwards
added
the
Needs Review
Sprint-3/alarmclock/alarmclock.js
Outdated
| const minutes = Math.floor(time / 60) | ||
| .toString() | ||
| .padStart(2, "0"); // calculate the minutes and format it to 2 digits | ||
| const seconds = (time % 60).toString().padStart(2, "0"); // calculate the seconds and format it to 2 digits |
There was a problem hiding this comment.
In general, the code looks good. here is something you could improve. const minutes = Math.floor(timeInSeconds / 60)
.toString()
.padStart(2, "0"); this has been used multiple time, so considering use a helper function to abstract this.
There was a problem hiding this comment.
oh yes of course, I was so busy being focused on getting the pull request completed I totally forgot to refactor the code :( - I have made the changes now and it is much better, thank you!
| // check for invalid inputs and update the <h1> element with an error message if the input is invalid | ||
| if (!Number.isInteger(timeInSeconds) || timeInSeconds < 0) { | ||
| alert("Please enter a valid number of seconds!"); | ||
| return; // stop the code if the input is invalid |
There was a problem hiding this comment.
if (typeof timeInSeconds !== "number" || timeInSeconds < 0) {
throw new Error("Invalid input: timeInSeconds must be a non-negative number");
} you could promote will a more specific alert message.
There was a problem hiding this comment.
I completely understand where you are coming from there. I suppose when I was writing the invalid input validation I was thinking along the lines of the UX for the user - I considered the system error approach but I believe it would crash the app. I have adjusted the message, however; it is now alert("Please enter a whole number of seconds (like 60 for 1 minute)")
I am curious to know which would be the better approach though in a real-world project- send an alert to the user or throw a system error?
Thank you :)
|
i think both should be okay, really depands on the context. in the current one, alert would be better but the example i threw was just to show the alert information could be more informative to user. |
|
it's a bit annoying as i cant' change the label. |
illicitonion
added
Reviewed
CameronDowner
added
Complete
Learners, PR Template
Self checklist
Changelist
Added user functionality to the Alarm Clock App using Javascript.
Questions
Ask any questions you have for your reviewer.