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 functionfunction 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();}1 Answer1
First is variable names. Most of them make sense... exceptac. Everything is so verbose, butac doesn't really make sense at first glance. One needs to read the code to find outac is actually a timerac = setTimeout(_f,interval);.
Next thing I notice is that all your actions seem to doprint. The problem here is that the functions are implicitly changing state by callingprint. That is, calling out any of the functions implicitly changes the value ofdisplayMinutes,displaySeconds as well as some element's text. Best ifadd,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 useslice 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 onsetTimeout 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 usingsetTimeout 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 usesetInterval, 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 usingDate.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 liketimerInstance.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);You mustlog in to answer this question.
Explore related questions
See similar questions with these tags.

