Javascript: timer function

Question

This is a function I made for a timer that counts down and performs a callback at the end. It seems to work fine, but I think the code is not very clean and has some unnecessary bits; also I would like some notes on readability. The code:

//timer object function
function Timer (element, callback) {

	var ac, minutes, seconds, finalTimeInSeconds, displayMinutes, displaySeconds, interval = 1000, self = this, timeLeftToNextSecond = 1000, running = false;
	
	this.set = function(inputMinutes, inputSeconds) {
		
		finalTimeInSeconds = inputMinutes * 60 + inputSeconds;
		minutes = (Math.floor(finalTimeInSeconds / 60));
		seconds = finalTimeInSeconds % 60;
		
		this.print();
	}
	
	this.add = function(inputMinutes, inputSeconds) {
	
		finalTimeInSeconds += inputMinutes * 60 + inputSeconds;
		finalTimeInSeconds = (finalTimeInSeconds < 0) ? 0 : finalTimeInSeconds;
		minutes = (Math.floor(finalTimeInSeconds / 60));
		seconds = finalTimeInSeconds % 60;
		
		this.print();
	}
	
	this.subtract = function(inputMinutes, inputSeconds) {

		finalTimeInSeconds -= inputMinutes * 60 + inputSeconds;
		if(finalTimeInSeconds < 0) {callback()}
		finalTimeInSeconds = (finalTimeInSeconds < 0) ? 0 : finalTimeInSeconds;
		minutes = (Math.floor(finalTimeInSeconds / 60));
		seconds = finalTimeInSeconds % 60;
		this.print();
	}
	
	this.reset = function() {
		
		this.set(0,0);
	}
	
	this.print = function() {
	
		displayMinutes = (minutes.toString().length == 1) ? "0" + minutes : minutes;	//ternary operator: adds a zero to the beggining 
		displaySeconds = (seconds.toString().length == 1) ? "0" + seconds : seconds;	//of the number if it has only one caracter.
		
		$(element).text(displayMinutes + ":" + displaySeconds);
	}
	
	this.run = function() {
		
		if (running == false) {
			running = true;
			
			var _f = function() {
				secondStarted = new Date;
				self.subtract(0, 1);
				interval = 1000;
				
				var theColorIs = $(element).css("color");
				ac = setTimeout(_f,interval);
				
				
			}
			ac = setTimeout(_f, interval);
			
			
		}
	}
	
	this.stop = function() {
		
		if (running == true) {
			running = false;
			console.log(this + "(" + element + ") was stopped");
			stopped = new Date;
			interval = 1000 - (stopped - secondStarted);
			clearTimeout(ac);
		}
	}
	
	this.getTotalTimeInSeconds = function() {
		

		return finalTimeInSeconds;
	}
	
	this.reset();
	
}

Answer 1

First is variable names. Most of them make sense... except ac. Everything is so verbose, but ac doesn't really make sense at first glance. One needs to read the code to find out ac is actually a timer ac = setTimeout(_f,interval);.

Next thing I notice is that all your actions seem to do print. The problem here is that the functions are implicitly changing state by calling print. That is, calling out any of the functions implicitly changes the value of displayMinutes, displaySeconds as well as some element's text. Best if add, subtract and the others actually just add and subtract. Leave the printing out of them, and do it separately.

displayMinutes = (minutes.toString().length == 1) ? "0" + minutes : minutes;  //ternary operator: adds a zero to the beggining 
displaySeconds = (seconds.toString().length == 1) ? "0" + seconds : seconds;  //of the number if it has only one caracter.

Looks like you're trying to convert numbers into strings padded by zeroes, if needed be. A neat trick to convert numbers into strings is to just add a string before it. Type coercion does the rest for you. Then you can use slice with a negative index to slice starting from the end of the string.

// We assume all numbers are at least 1 digit. Thus we need one zero and 2
// digits to slice off from the right. If it were 3 digit padded numbers,
// we'd need two zeroes and 3 digits to slice off from the right.
var paddedTwoDigitNumber = ('0' + number).slice(-2);

Now I notice your timer relies on setTimeout to count. There are 2 problems here. First, native JS timers aren't precise. That means putting 1000ms delay guarantees that it executes at least 1000ms, but not exactly at 1000. This means, every "tick" may drift off a few ms. The next problem is that you're using setTimeout for each tick. The problem is that internally, it keeps queueing a function as a callback, and every execution, it gets popped off.

A better approach would be to use setInterval, so that you only queue your tick once and you don't have to rely on some recursion call. Next is instead of relying on the timer ticks, you should get the real time using Date.now(). Every timer tick, you store the time andyou check the real time, if a full second has elapsed since last timer tick. Only then do you update your timer. There will always be "drift" on each interval, so you should take that into account as well.

Not sure if you should pepper your variables in "InSeconds" and that's because I suggest you should work with milliseconds in JS. Most Date functions return millisecond time. Also, it's easier to convert milliseconds to seconds (a matter of division) rather than convert seconds to milliseconds (a code overhaul).

I notice that your code relies on jQuery (I see $(element).text()). I think it's best that either jQuery stays out of it (your timer is merely a timer library) or you make it a jQuery plugin. Either way is fine.

If you go with the pure timer route, I suggest you let Timer be an instance of an event emitter class (There's tons of them online. I even created one in 21 lines.) That way, your timer can just emit events when interesting things happen (on start, on stop etc.) and the DOM manipulation code is outside the Timer. You could just have something like timerInstance.on('complete', fnToExecuteOnComplete). jQuery is no longer a concern for the Timer code. It's external.

If you make it a jQuery plugin, it's a matter of doing the following to your code. There are a lot of approaches in building jQuery plugins online. Find one that suits your taste.

$.fn.timer = function(){
  // "this" here is a reference to the element, not wrapped in jQuery.
  // Then put your operations here.
}

// To use, it would be something like
$(element).timer({ mins: 60, seconds: 00 });

// To operate
$(element).timer('add', 1, 2);