Stop and run again timer via pure javascript

Question

I want to make a simple countdown timer which can be set by + or - and also it can be stopped and run by clicking on itself. My problem is when it is stopped and then runs it shows NAN for the first number. I suppose it is because of setTimer function but I don't know how to fix that.Here is my code:

var x = document.getElementsByClassName('session');
var seconds = 60;
var session;
var t;
var on = true;
var minutes = 1;

for (var i = 0; i < x.length; i++) {
  x[i].innerHTML = minutes;
}

function increase() {
  minutes++;
  for (var i = 0; i < x.length; i++) {
    x[i].innerHTML = minutes;
  }
}

function decrease() {
  minutes--;
  for (var i = 0; i < x.length; i++) {
    if (x[i].innerHTML > 0) {
      x[i].innerHTML = minutes;
    }
  }
}

function setSession() {
  session = x[1].innerHTML - 1;
}

function timer() {
  if (seconds > 0) {
    seconds--;
    if (seconds == 0 && session > 0) {
      session--;
      seconds = 60;
    }
  }
  x[1].innerHTML = session + ':' + seconds;
}


function stoptimer() {
  clearInterval(t);
}


function general() {
  if (on) {
    on = false;
    t = setInterval(timer, 100);
  } else {
    on = true;
    stoptimer();
  }
}

<div class='session'></div>
<div id='increase' onclick='decrease()'>-</div>
<div id='increase' onclick='increase()'>+</div>
<div class='session' onclick='setSession();general()'></div>

Answer 1

You set

x[1].innerHTML = session + ':' + seconds;

and then try to calculate that as

session = x[1].innerHTML - 1;

You need to either put the session:seconds in another place or do

session = parseInt(x[1].innerHTML,10) - 1;

Answer 2

You shouldn't be setting session from the html entity. Basically this creates issues with parsing and could potentially break your code. Also, you subtract one every time you get this value, throwing a spanner in the works.

I re-shuffled your code a bit and added some notes, take a look:

var x = document.getElementsByClassName('session');
var seconds = 0;
var session;
var timer; // give this a useful name
var timerRunning = false; // give this a useful name
var minutes = 1;



function updateMinutes(newMinutes){

    if (timerRunning){
        return; // do not allow updating of countdown while count down is running
    }

    if(newMinutes !== undefined){ // allow this function to be called without any parameters
        minutes = newMinutes; 
    }

    if(minutes < 1){
        minutes = 1; //set your minimum allowed value
    }
    if(minutes > 99999){
        minutes = 99999; //could also have some sort of maximum;
    }

    for (var i = 0; i < x.length; i++) {
      x[i].innerHTML = minutes;
    }

    session = minutes; // now this can only be set while timer is not running, so no need to get it from the html
    //also, i would let this start at the exact same value as minutes, and have seconds start at zero
}

updateMinutes(); // call this now to initialise the display

function increase() {
  updateMinutes(minutes + 1);
}

function decrease() {
  updateMinutes(minutes - 1);
}

function timer_tick() {
  if (seconds > 0) {
    seconds--;
    if (seconds == -1 && session > 0) { //because of where you've positioned your logic, this should check for negative one, no zero, otherwise you'll never display a zero seconds value
      session--;
      seconds = 59; //when a timer clocks over it goes to 59
    }
  }
  if (session > 0 || seconds > 0){
      x[1].innerHTML = session + ':' + seconds;
  }
  else{// you need to detect the ending
      x[1].innerHTML = "Finished!!";
  }
}

function timer_stop() {
    clearInterval(timer);
}

function timer_start(){
    timer = setInterval(timer_tick, 1000);
}

function timer_toggle() { //give these timer functions consistent names
  if (!timerRunning) {
    timer_start();
  } else {
    timer_stop();
  }
  timerRunning = !timerRunning; //just flip the boolean
}

Answer 3

Ok, I would propose another approach. Use a class for your timer, like this:

function MyTimer(htmlEl) {
  this.sec = 0;
  this.min = 0;
  this.elt = htmlEl;
}

MyTimer.prototype.set = function(m) {
  this.min = m;
  this.display();
  var self = this;
  this._dec = function() {
    self.sec--;
    if (self.sec < 0) {
      if (self.min == 0) {
        self.stop();
      } else {
        self.min -= 1;
        self.sec = 59;
      }
    }
    self.display();
  }
}

MyTimer.prototype.display = function() {
  this.elt.innerHTML = this.min + ":" + this.sec;
}



MyTimer.prototype.toggle = function() {
  if (this.interval) {
    this.stop();
    this.interval = undefined;
  } else this.start();
}

MyTimer.prototype.start = function() {
  this.interval = setInterval(this._dec, 100);
};

MyTimer.prototype.stop = function() {
  clearInterval(this.interval);
};

Then, you can use it like this:

window.onload = init;

var minutes, x, timer;

function init() {
  x = document.getElementsByClassName('session');
  timer = new MyTimer(x[1]);
  minutes = 0;
}

function increase() {
  minutes += 1;
  x[0].innerHTML = minutes;
  timer.set(minutes);
}

function decrease() {
  minutes -= 1;
  x[0].innerHTML = minutes;
  timer.set(minutes);
}


function general() {
  timer.toggle();
}

The only change in your html is to remove the setSession call:

<div id='timer' onclick='general()'></div>

This code is more clear. You encapsulate the logic and the min/sec in an object, which is reusable. Here is a working fiddle : https://jsfiddle.net/Shitsu/zs7osc59/.

Answer 4

The origin of your problem is in the code

    session = x[1].innerHTML - 1;

Let's re-visit the purpose of keeping of the variable 'session'. I guess it is to keep the value of the maximum value, the upper limit of the minutes, from where to start counting, right? On the other hand, the 'minutes' variable is to keep the temporary value of the minutes. What confused you here is that you've used 'minutes' in place of it's upper limit (what actually is the session's role), in this code for example

    function increase() {
        minutes++;
        for (var i = 0; i < x.length; i++) {
           x[i].innerHTML = minutes;
        }
    }

see, you are updating html by the value of 'minutes', and later you are reading that value into 'session' by that evil code:

    session = x[1].innerHTML - 1;

So, why? Why you need to update the value of 'your' variable from html? You should only update the html according to the value of session var and not vice versa. Let's go on and make life simpler... Let's keep the temporary value of minutes in 'minutes', let's also keep the upper limit in a variable session and, please, let's rename it to maxMinutes. Let's update the 'maxMinutes' only when user clicks '+' or '-' (NEVER from html).

<!DOCTYPE html>
<html>
<head>
<script src="https://code.jquery.com/jquery-1.9.1.min.js"></script>
</head>
<body>

<script>

var x = document.getElementsByClassName('session');
var maxMinutes = 0;
var minutes = maxMinutes;
var seconds = 0;
var timer;
var on = false;

function increase() {
  if(on) {
    return;
  }
  maxMinutes++;
  minutes = maxMinutes;
  x[0].innerHTML = maxMinutes;
  x[1].innerHTML = minutes;
 }

function decrease() {
  if(on) {
    return;
  }
  if(maxMinutes == 0)
  {
    return;
  }

  maxMinutes--;

  minutes=maxMinutes;

  x[0].innerHTML = maxMinutes;
  x[1].innerHTML = minutes;

}

function periodicRun() {
  if (seconds > 0) {
    seconds--;
  }
  else if(seconds == 0)
  {
   if(minutes > 0) {
      minutes--;
      seconds = 60;
    }
    else
    {
      stopTimer(timer);
      return;
    }
  }
  x[1].innerHTML = minutes + ':' + seconds;
}


function stoptimer() {
  clearInterval(timer);
}


function general() {
  if (on) {
    on = false;
    stoptimer();
  } else {
    on = true;
    timer = setInterval(periodicRun, 500);
  }
}

$(document).ready(function(){
  increase();
});

</script>

<div class='session'></div>
<div id='increase' onclick='decrease()'>-</div>
<div id='increase' onclick='increase()'>+</div>
<div class='session' onclick='general()'></div>

</body>
</html>

Note, that the only place where the maxLimits get's assigned a value is in increase() and decrease(). The 'minutes' and html are in their turn being updated according to maxMinutes. Good Luck!