HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Creating a Tomato Timer

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
creatingtimertomato

Problem

I have completed a FreeCodeCamp exercise to create a tomato timer and chosen to do so with vanilla Javascript. Although it works, I don't like in how many places I am checking against two state variables onBreak and isPaused in many areas of the application.



(function() {

"use strict";

// Initialisation
var breakLength = 300;
var sessionLength = 1500;
var clockTime = sessionLength;
var timerPaused = true;
var onBreak = false;
var CLOCK_PIXEL_HEIGHT = 286;

// Setters / Getters
function setBreakLength(newLength) {
breakLength = (newLength

PomodoroTimer




BREAK LENGTH
SESSION LENGTH



- 3
+


- 7
+






Session
7






RESET


`



See the end product: http://danielrob.github.io/pomodoro-timer/site/

My key question: is there a way I could reduce my global variables or manage the four possible states [(on session, timer running), (on session, timer paused), (on break, timer running), (on break, timer paused) ] more elegantly?

Solution

A few notes on your current code:

  • IIFE and "use strict" - excellent!



  • Use of onclick - less excellent. Better to use addEventListener and keep javascript and HTML separate.



  • You can skip the DOMContentLoaded handling by simply putting your script tag at the end of the body tag. That way, all the preceding DOM content will have been loaded when the script starts executing.



Other than that, the icky bits are the state-keeping that you yourself mentioned.

I should add that the UI makes more sense on the page you linked, than it does in the unstyled snippet. Still, even with styling, it wasn't clear what I was supposed to click to actually start the timer. Consider making the big button more "button-like". I know buttons these days are often flat-looking circles, but this flat circle is so large it fails to register as a button - looks more like a purely aesthetic thing. Anyway, that's neither here nor there.

Now, suggestions. I'd suggest wrapping the timers in objects which can encapsulate state for you:



function Timer(duration) {
this.duration = duration;
this.remaining = duration;

this.onupdate = function () {};
this.onfinish = function () {};
}

Timer.prototype = {
isRunning: function () {
return !!this.timer;
},

hasFinished: function () {
return this.remaining === 0;
},

reset: function () {
this.stop();
this.remaining = this.duration;
},

start: function () {
if(this.isRunning() || this.hasFinished()) {
return;
}
this.lastTime = Date.now();
this.timer = setInterval(this.update.bind(this), 100);
},

stop: function () {
clearInterval(this.timer);
this.timer = null;
},

update: function () {
var delta = Date.now() - this.lastTime;
this.remaining = Math.max(0, this.remaining - delta);
this.lastTime = Date.now();
this.onupdate();

if(this.remaining
0:10

Toggle timer



You could also easily add a
progress method to the prototype which return a 0-1 number indicating how far in the session/break you are.

As for keeping track of sessions/breaks. I'd create 2 of those
Timer instances. You can add a name argument to the constructor, which you can display on-update, if you want.

Point is, if the session timer
hasFinished(), then check the break timer. If it too hasFinished()`, then reset and start the session timer again.

Context

StackExchange Code Review Q#108907, answer score: 4

Revisions (0)

No revisions yet.