A well-organized system #3
Conversation
| const currentTimeInfo = new TimeInformation( | ||
| working, | ||
| workTime, | ||
| restTime, | ||
| timeLeft | ||
| ); | ||
|
|
||
| theText = standardText(timeFormatter.textFrom(currentTimeInfo)); |
There was a problem hiding this comment.
This section of code now adheres to the Open-Closed Principle.
| if (resetTimer) { | ||
| buttons.forEach((b) => b.remove()); | ||
| endTime = getTargetTime(working ? workTime : restTime); | ||
| buttons = [ | ||
| makeButton( | ||
| "Reset", | ||
| (currentButtonsAmount = modes.length), | ||
| resetProgram | ||
| ), | ||
| ]; | ||
|
|
||
| soundEffect.play(); | ||
|
|
||
| resetTimer = false; | ||
| } |
There was a problem hiding this comment.
This could be better, we should strive for separation of concerns.
There was a problem hiding this comment.
It would be ideal to make a function call instead of setting a global variable used in another function, maybe making a resetTimer function would do the job.
| windowWidth / 2 - buttonWidth / 2, | ||
| windowHeight / 2 + | ||
| idx * buttonHeight - | ||
| (modes.length * buttonHeight) / 2 |
There was a problem hiding this comment.
This is complex logic that would be good to describe in more detail.
| ) | ||
| ) | ||
| .map((b) => b.size(buttonWidth, buttonHeight)) | ||
| .map((b, idx) => b.mousePressed(() => setMode(modes[idx]))); |
There was a problem hiding this comment.
This may not be the best solution, but at least we depend only on a parameter.
| button.position( | ||
| windowWidth / 2 - buttonWidth / 2, | ||
| windowHeight / 2 + | ||
| currentButtonsAmount * buttonHeight - | ||
| (totalButtons * buttonHeight) / 2 | ||
| ); |
There was a problem hiding this comment.
This logic looks to be duplicated in makeButtonsFromModes. Needless to say, this is not ideal.
| <body> | ||
| <script src="https://cdn.jsdelivr.net/npm/[email protected]/lib/p5.js"></script> | ||
| <script src="static/js/p5.sound.js"></script> | ||
| <script type="module" src="static/js/main.js"></script> |
There was a problem hiding this comment.
Separation of concerns - I called this one "Code-Interface Segregation".
|
|
||
| function resetProgram() { | ||
| //executes once to reset program | ||
| buttons.forEach((b) => b.remove()); |
There was a problem hiding this comment.
I do see this call repeated in a few places. It's probably wise to extract this into its own function.
| var timerRunning = false; | ||
| var buttons = []; | ||
| var currentWorkTime, currentRestTime; | ||
| var working = true; //false=resting | ||
| var currentButtonsAmount = 0; | ||
| var buttonWidth, buttonHeight; // Should be constant, but due to the way processing works, must be set in `setup()`. | ||
| var endTime; | ||
| var resetTimer = false; //for pomodoro screen | ||
| var theText; //for pomodoro screen - global | ||
| var timeLeft; | ||
| var soundEffect; // Should be constant. |
There was a problem hiding this comment.
All of these should be able to be turned into let without unexpected behavior. Ideally we would remove as many variables as possible.
There was a problem hiding this comment.
Is this a dataclass? Could this design be better?
| return { | ||
| hours: Math.floor(diff / 60 / 60), | ||
| minutes: Math.floor(diff / 60), | ||
| seconds: diff.toFixed(2), | ||
| }; |
There was a problem hiding this comment.
If we made this into an actual class, we could get the benefit of better type inference in the IDE and reader clarity. A named class is always clearer than Object.
Structural redesign, modularization, and separation of concerns. Still works just the same (except the resting screen is a less harsh blue and I added a new timer mode).