-
-
Notifications
You must be signed in to change notification settings - Fork 149
Sheffield|May-2025|Sheida Shabankari|Module-Data -Groups| Sprint-3|Alarm Clock #631
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Code works fine if a user only clicks the "Set Alarm" button once.
However, if the user enters a time and then clicks the "Set Alarm" button multiple times, the countdown clock will not display properly.
Can you fix the issue? -
Some of the function definitions appear twice. Can you remove the unwanted copies?
-
We should respect instructions like
DO NOT EDIT BELOW HERE
; it is usually there for a reason. If you are curious about why, you can ask ChatGPTWhy should programmers respect "DO NOT EDIT BELOW HERE" instruction in a file?
(You don't have to undo the changes you made this time, I just want to raise your awareness)
Sprint-3/alarmclock/alarmclock.js
Outdated
@@ -1,5 +1,98 @@ | |||
function setAlarm() {} | |||
var flashColor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use var
instead of let
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used var out of old habit because I used to work with it before, and I wasn’t really paying attention to why I was using it.
if (inputTime <= 0 || isNaN(inputTime)) { | ||
alert("invalid input!!!"); | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some unusual input values that can make your app behave abnormally can still pass this check. Can you add code to sanitise them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have already handled empty input, negative numbers, decimals, and zero values. Since the input field is defined as type 'number', non-numeric inputs are also prevented altogether. The only thing left, in my opinion, is controlling very large numbers. Do you think it's necessary to set a limit on the numeric value that can be entered? Also, I don’t have any other ideas about unusual inputs that might cause issues—if possible, could you please guide me on that?"
Sprint-3/alarmclock/alarmclock.js
Outdated
let min = String(Math.floor(inputTime / 60)).padStart(2, "0"); | ||
let sec = String(inputTime % 60).padStart(2, "0"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can consider declaring these two variables with const
because they won't be reassigned any value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure,you are right.I fixed it.
Sprint-3/alarmclock/alarmclock.js
Outdated
document.getElementById("stop").style.backgroundColor = "rgb(191, 115, 47)"; | ||
document.body.style.backgroundColor = "rgb(218, 178, 119)"; | ||
let flashColor=setInterval(() => { | ||
if(document.body.style.backgroundColor === "rgb(218, 178, 119)"){ | ||
document.body.style.backgroundColor = "white"; | ||
}else{ | ||
document.body.style.backgroundColor = "rgb(218, 178, 119)"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could consider preparing several CSS classes in a CSS file and then assign/remove CSS class to these element to change their appearance. This way, if we want to change the styles (including color), we don't have to modify the JS code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your guidance, I understand, but due to lack of time, I’m unable to make this change right now. If you don’t mind, and there’s no problem with it, I’d like to leave it as is for now.
I fixed the issues you mentioned. Now, even if the user clicks the “Set Alarm” button multiple times, the timer works properly . I also removed the duplicate functions . I asked ChatGPT about the DO NOT EDIT BELOW HERE part and now I get why we shouldn’t touch that section. Thanks for your guidance—it really helped me improve the code and learn new things. |
Sprint-3/alarmclock/alarmclock.js
Outdated
@@ -1,85 +1,26 @@ | |||
var flashColor; | |||
let flashColor, secondsInterval, inputTime; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
The
inputTime
in function setAlarm() is not related to theinputTime
parameter inupdateDisplayTime()
. In both functions, the variable/parameter is used locally, soinputTime
should not be declared at in the top-level scope. -
Also, since
inputTime
in only assigned once in functionsetAlarm()
, it could better be declared usingconst
inside that function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed the issue.
Sprint-3/alarmclock/alarmclock.js
Outdated
let remainingTime = inputTime; | ||
updateDisplayTime(remainingTime); | ||
|
||
secondsInterval=setInterval(() => { | ||
remainingTime--; | ||
updateDisplayTime(remainingTime); | ||
if (remainingTime === 0) { | ||
clearInterval(secondsInterval); | ||
playAlarm(); | ||
} | ||
}, 1000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation is a bit off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed it.
Sprint-3/quote-generator/quotes.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should not be included in the alarm-clock PR. Can you revert the change made to this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted the changes.
Self checklist
I have done all requirements for setting an alarm clock step by step and passed all the tests.