Creating a Tomato Timer

Question

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 <= 0) ? 0 : newLength;
     notify();
   }

   function setSessionLength(newLength) {
     sessionLength = (newLength <= 0) ? 0 : newLength;
     notify();
   }

   function setClockTime(secs) {
     clockTime = secs;
     notify();
   }

   function getClockTime() {
     return clockTime;
   }

   // Timer
   var intervalTimer;

   window.toggleTimer = function() {
     timerPaused = !timerPaused;
     if (!timerPaused) startTimer();
     else stopTimer();
   };

   function startTimer() {
     notify();
     intervalTimer = setInterval(tick, 1000);
   }

   function stopTimer() {
     clearInterval(intervalTimer);
   }

   function tick() {
     if (getClockTime() <= 0) {
       stopTimer();
       toggleBreak();
       return;
     }
     setClockTime(getClockTime() - 1);
   }

   function toggleBreak() {
     onBreak = !onBreak;
     setClockTime(onBreak ? breakLength : sessionLength);
     startTimer();
   }

   /*
     Controls
   */

   window.breakMinus = function() {
     incrementBreakLength(-60);
   };
   window.breakPlus = function() {
     incrementBreakLength(60);
   };
   window.sessionMinus = function() {
     incrementSessionLength(-60);
   };
   window.sessionPlus = function() {
     incrementSessionLength(60);
   };

   window.reset = function() {
     stopTimer();
     timerPaused = true;
     onBreak = false;
     setClockTime(sessionLength);
   };


   // Increment break length. If on break, and timer paused, reset clock to new length.
   function incrementBreakLength(increment) {
     setBreakLength(breakLength + increment);
     if (timerPaused && onBreak) {
       setClockTime(breakLength);
     }
   }

   // Increment session length. If in session, and timer paused, reset clock to new length.
   function incrementSessionLength(increment) {
     setSessionLength(sessionLength + increment);
     if (timerPaused && !onBreak) {
       setClockTime(sessionLength);
     }
   }

   /*
     Update View Function
   */
   var notify = (function() {
     var prevBreakLength;
     var prevSessionLength;

     return function() {
       // Just always update clock.
       document.getElementById('timer').innerHTML = formatClockDisplay();
       document.getElementById('title').innerHTML = onBreak ? 'Break' : 'Session';
       document.getElementById('fill').style.backgroundColor = onBreak ? 'red' : 'green';
       document.getElementById('fill').style.height = fillHeight();

       // Update Break Length
       if (prevBreakLength !== breakLength) {
         document.getElementById('break-length').innerHTML = breakLength /  60;
         prevBreakLength = breakLength;
       }

       // Update Session Length
       if (prevSessionLength !== sessionLength) {
         document.getElementById('session-length').innerHTML = sessionLength / 60;
         prevSessionLength = sessionLength;
       }
     };

   })();
   /*
     Helpers
   */
   function formatClockDisplay() {
     var time = getClockTime();
     var mins = Math.floor(time /  60);
     var secs = clockTime % 60;
     secs = secs <  10 ? '0' + secs : secs;
     return (timerPaused && time === sessionLength) ? mins : mins + ':' + secs;
   }

   var fillHeight = (function() {
     var fillBreakLength = breakLength;
     var fillSessionLength = sessionLength;
     var prevOnBreak = onBreak;

     return function() {
       if (timerPaused || prevOnBreak !== onBreak) {
         fillBreakLength = breakLength;
         fillSessionLength = sessionLength;
         prevOnBreak = onBreak;
       }
       var period = onBreak ? fillBreakLength : fillSessionLength;
       var left = getClockTime();
       return (period - left) /  period * CLOCK_PIXEL_HEIGHT + 'px';
     };

   })();

   // init view
   document.addEventListener("DOMContentLoaded", function() {
     notify();
   }, false);

 })();

<body>
  <h1>PomodoroTimer</h1>

  <div id="control-panel">
    <table align="center">
      <tr>
        <td>BREAK LENGTH</td>
        <td>SESSION LENGTH</td>
      </tr>
      <tr>
        <td>
          <button onclick="breakMinus()">-</button> <span id="break-length">3</span> 
          <button onclick="breakPlus()">+</button>
        </td>
        <td>
          <button onclick="sessionMinus()">-</button> <span id="session-length">7</span> 
          <button onclick="sessionPlus()">+</button>
        </td>
      </tr>
    </table>
  </div>
  <div id="pomodoro" onclick="toggleTimer()">
    <div class="pomoinner">
      <p id="title">Session</p>
      <p id="timer">7</p>
      <span id="fill"></span>
    </div>
  </div>
  <div id="reset">
    <p>
      <button onclick="reset()">RESET</button>
    </p>
  </div>
</body>

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?

Answer 1

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) {
      this.stop();
      this.remaining = 0;
      this.running = false;
      this.onfinish();
    }
  },
  
  toString: function () {
    var total = this.remaining / 1000 | 0,
        minutes = total / 60 | 0,
        seconds = total % 60;
    
    return String(minutes) + ":" + ("00" + seconds).slice(-2);
  }
};

// Test (just a 10sec timer)

var t = new Timer(10*1000);

t.onupdate = function () {
  document.getElementById("time").innerHTML = this.toString();
};

t.onfinish = function () {
  document.getElementById("toggle").disabled = true;
};

document.getElementById("toggle").addEventListener("click", function () {
  var clock = document.getElementById("time");
  if(t.isRunning()) {
    clock.style.color = "gray";
    t.stop();
  } else {
    clock.style.color = "black";
    t.start();
  }
}, false);

<span id="time">0:10</span>
<br>
<button id="toggle">Toggle timer</button>

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.