London | 26-ITP-May | Alex Jamshidi | Sprint 3 | Alarm Clock #1227
London | 26-ITP-May | Alex Jamshidi | Sprint 3 | Alarm Clock #1227Alex-Jamshidi wants to merge 6 commits into
Conversation
Alex-Jamshidi
added
📅 Sprint 3
illicitonion
left a comment
illicitonion
left a comment
There was a problem hiding this comment.
This generally looks good, but I left a few things to think about / work on.
Good job extracting away the duplication of showing a time into a reusable function.
| let timer; | ||
|
|
||
| function setAlarm() { | ||
| timeSet = document.getElementById("alarmSet").value; |
There was a problem hiding this comment.
What's the scope of this variable? Is there a better scope for it?
There was a problem hiding this comment.
this would have been a global variable without declaring it
changed to 'const' to restrict scope to function
| if (time > 0) { | ||
| countdown(time); | ||
| } |
There was a problem hiding this comment.
Why do you have this if block? What does it do? When will it run?
There was a problem hiding this comment.
It doesn't, this was old code that I was using to stop the countdown, but later added into the setInterval function.
Redundant code - removed.
| }, 1000); | ||
| } | ||
|
|
||
| function UpdateShownTime(timeRemaining) { |
There was a problem hiding this comment.
The name of this function doesn't follow the same pattern of the other function names - can you make them consistent?
There was a problem hiding this comment.
removed capitalisation at start
illicitonion
added
Reviewed
|
|
||
| function setAlarm() { | ||
| const timeSet = document.getElementById("alarmSet").value; | ||
| const time = countdown(timeSet); |
There was a problem hiding this comment.
What's the point of this time variable now?
There was a problem hiding this comment.
have removed variable assignment as after deleting code snippet before, it is no longer used. countdown(timeSet) is run without result assigned to variable.
Alex-Jamshidi
added
Needs Review
illicitonion
added
Complete
|
Looks good, thank you! |
2 participants
Learners, PR Template
Self checklist
Changelist
created alarm clock code