Conversation
pmayd
left a comment
pmayd
left a comment
There was a problem hiding this comment.
Solid work, but room for improvement and refactoring :)
| from datetime import datetime, timedelta | ||
| from time import sleep | ||
|
|
||
| if platform.system() == "Windows": |
There was a problem hiding this comment.
Another way of doing it, platform acnostic, is to just do the import and catch a ModuleNotFoundError with a try except
52/zambam5/timer.py
Outdated
| from winsound import Beep | ||
|
|
||
|
|
||
| def pomodoro_timer(duration: int = 20, beep: bool = False): |
There was a problem hiding this comment.
When you are using type hints, annotate the whole function and that means also the return value.
52/zambam5/timer.py
Outdated
| Take a duration and begin a timer loop | ||
| """ | ||
| duration_delta = timedelta(minutes=duration) | ||
| d = datetime.now() |
There was a problem hiding this comment.
Variable names should be speaking and have a meaning, this is not a good name :)
52/zambam5/timer.py
Outdated
| from winsound import Beep | ||
|
|
||
|
|
||
| def pomodoro_timer(duration: int = 20, beep: bool = False): |
There was a problem hiding this comment.
Better name for duration would be minutes in this case
| print("Starting timer " + str(d), "\r\nTimer will stop at " + str(d_)) | ||
| sleep(duration_delta.seconds) | ||
| if beep: | ||
| Beep(frequency=500, duration=50) |
There was a problem hiding this comment.
This code is problematic because You do not check for the platform again, so this will fail ok all platforms where Beep is not available but beep is set to True
| beep = False | ||
| else: | ||
| beep = True | ||
| else: |
There was a problem hiding this comment.
Set beep to false before the user input and only set it to True when the user says yes, saves you one else branch
52/zambam5/timer.py
Outdated
| again = input( | ||
| 'Do you want to start the timer again? Answer "yes" or "no" only ' | ||
| ) | ||
| if again == "yes": |
There was a problem hiding this comment.
You don't have to check here, you can completely omit this branch, only check for when you want to do something. Here you want to do something if the user does not want to continue so only check for "no"
pmayd
left a comment
pmayd
left a comment
There was a problem hiding this comment.
Solid work but room for improvement:)
Needed to change a yes to a no
2 participants
Difficulty level (1-10): [1]
Estimated time spent (hours): [1]
Completed (yes/no): [Yes]
I stretched my coding skills (if yes what did you learn?): [I chose to figure out how to make a notification sound when the timer ends, something I had never considered doing before.]
Other feedback (what can we improve?): []