Javascript timing class

Question

I've recently came across many inconsistencies in a code which I had to maintain and develop. Mainly in ajax and timing/interval functions. I'm quite a novice in the ways of JS but tried my best to standarize those into one timer class with passable functions for specific inpage changes since It was supposed to be universal app-wide. The class is working nicely and I'm quite happy with it but would love some tips on how to improve it further.

/**
* Timer class
* Ajax functions have to set self.stillPending before start and zero it after the responce has been received for the class to work properly.
* @param function startFunc Function used when starting the timer
* @param function updateFunc Function used every cycle
* @param function  endFunc Function used when stopping the timer
* @param integer   refreshDelay Delay between update cycles (in seconds) (default 5)
* @param function timerFunc Function used every second (to update GUI timers etc. SHOULDNT EXECUTE ANYTHING PROCESSING HEAVY I.E AJAX)
* @param boolean pregeneration Execute one update before the timer start to predownload data
* @returns Timer Class Object
*/
class Timer {
    constructor(startFunc, updateFunc, endFunc, refreshDelay = 5, timerFunc, pregeneration = false) {
        this.startFunc = startFunc;
        this.endFunc = endFunc;
        this.updateFunc = updateFunc;
        this.timerFunc = timerFunc;
        this.refreshDelay = refreshDelay;
        this.time = 0;
        this.timeCycle;
        this.cycle;
        this.tempData;
        this.stillPending = 0;
        this.isDisabled = 0;
        this.inversion = 0;
        this.pregeneration = pregeneration;
    }

    updateData() {
        var self = this;
        if (self.updateFunc !== null) {
            if (self.stillPending === 0) {
                self.updateFunc();
            }
        }
    }

    restartCycle() {
        var self = this;
        if (self.cycle > 0) {
            self.stopCycle();
        }
        self.startCycle();
    }

    startCycle() {
        var self = this;
        self.isDisabled = 0;
        if (!self.cycle) {
            if (self.startFunc !== null) {
                self.startFunc();
            }
            if (self.refreshDelay !== 1) {
                self.timerCycle = setInterval(function () {
                    self.time = (self.time + 1);
                    if (self.timerFunc !== null) {
                        self.timerFunc();
                    }
                }, 1000);
            }
            if (self.pregeneration === true) {
                self.updateData();
            }
            self.cycle = setInterval(function () {
                if (self.updateFunc !== null) {
                    self.updateData();
                }
                if (self.refreshDelay === 1) {
                    self.time = (self.time + 1);
                    if (self.timerFunc !== null) {
                        self.timerFunc();
                    }
                }
            }, (self.refreshDelay * 1000));
        }
    }

    stopCycle() {
        var self = this;
        if (self.cycle) {
            if (self.endFunc !== null) {
                self.endFunc();
            }
            self.isDisabled = 1;
            clearInterval(self.cycle);
            clearInterval(self.timeCycle);
            self.cycle = null;
            self.timeCycle = null;
        }
    }
}

Its intended example usage would be

var statisticsTimer = new Timer(null, function(){
    //updatefunc
    var self = this;
    self.stillPending = 1;
    [..]ajax[..].then(function(data){
        [..]stuff to do[..]
        self.stillPending = 0;
    });
}, null, 30, null, true);

$(document).ready(function(){
    statisticsTimer.startCycle();
});

Which would pregenerate data before interval with set update function and execute it every cycle ie every 30sec. Other possible unpassed functions would mainly do other magic if needed on page such as predefine containers so the update function actually just updates data without any heavy html editing. tempData is for some dynamic data holding within class between cycles or so.

Answer 1

Looking into this, it appears to be fairly straightforward. There are a few things that I would advise changing.

1. Don't include statements that do nothing. In your constructor, none of the following lines do anything.

   this.timeCycle;
   this.cycle;
   this.tempData;
   

2. Default parameters should be included after all required parameters. This is to avoid passing in extra information that is not necessary. In other words,

   constructor(startFunc, updateFunc, endFunc, refreshDelay = 5, timerFunc, pregeneration = false)
   

   should be changed to

   constructor(startFunc, updateFunc, endFunc, timerFunc, refreshDelay = 5, pregeneration = false)
   

3. I'm unsure of the use of having both an update and a timer function. I believe this adds unnecessary complication to your code that could be avoided by simply creating two timer instances.

4. Adding Func to the function parameters doesn't help when reading the code so long as the types are properly documented. I personally would remove them as it makes the code more awkward to read out loud.

5. Consider passing in an options object instead of multiple options. I'm guessing that the constructor was formated with refreshDelay before timerFunc because timerFunc is used less often. A better solution is to simply use an object. An example of this that makes everything but update optional is the following

   constructor(update, {start, end, refreshDelay = 5, pregeneration = false} = {})
   

6. Using an instance variable to check if a function is still working is far from an ideal solution. Since you are already using promises for ajax, why not use them here too? Have update return a promise and wait for that promise to be resolved before starting the next cycle. For functions which don't need to wait for ajax responses, also allow the update function to simply return and immediately queue the next cycle.

7. You require that unused parameters be strictly null. This isn't a good idea as it makes it harder to use the class. A better solution is to simply check if it is truthy. With this change, users of the class can simply not include the parameter. Alternatively, it might be a good idea to check to ensure that function are actually functions with typeof.

   if (this.timerFunc) {
       this.timerFunc();
   }
   

8. There's no need to use var self = this for most of the functions. It is only needed for those functions which include closures. It can be eliminated entirely if you use Arrow functions instead.

9. JavaScript isn't C. JavaScript has a boolean type, isDisabled should be true or false instead of 1 or 0.

10. I might be missing something, but isn't the pregeneration option made redundant by assigning start?

---

With all this in mind, here is how I would recommend implementing the Timer class.

As far as I can tell, there is no need to keep the concept of cycles. In general, the timer's update function will keep track of the state if necessary, there's no need for the timer class to do it. For this reason I have dropped the cycle counter.

/**
 * Timer class to manage events which should be run on an interval.
 */
class Timer {
    /**
     * 
     * @param {Function} onTick The function to call whenever the interval has elapsed. This function may return a promise. If a promise is returned the next tick will not be scheduled until the promise resolves.
     * @param {Object} [options] Any options to modify the instance behavior. By default, onStart and onStop do nothing, and refreshDelay is 5.
     * @param {Function} options.onStart A function to call when the timer is started. If onStart returns a promise, the class will wait until the promise has resolved before starting the timer.
     * @param {Function} options.onStop A function to call when the timer has been stopped.
     * @param {number} options.refreshDelay The minimum number of seconds between ticks.
     */
    constructor(onTick, {
        onStart = () => {},
        onStop = () => {},
        refreshDelay = 5,
    } = {}) {
        if (typeof onTick != 'function') throw Error('onTick must be a function');
        if (typeof onStart != 'function') throw Error('onStart must be a function');
        if (typeof onStop != 'function') throw Error('onStop must be a function');
        // Number.isNaN is more strict than isNaN as it requires that the type be a number.
        if (Number.isNaN(refreshDelay)) throw Error('refreshDelay must be a number');
        this.onTick = onTick;
        this.onStart = onStart;
        this.onStop = onStop;
        this.interval = refreshDelay * 1000;

        this.enabled = false;
        this.timeoutId = null;
    }

    start() {
        // You may wish to instead just return, but throwing an error immediately shows a problem in the code.
        if (this.enabled) throw Error("Timer has already started.");
        this.enabled = true;
        Promise.resolve(this.onStart()).then(() => {
            this.timeoutId = setTimeout(() => this._tick(), this.interval);
        });
    }

    stop() {
        if (!this.enabled) throw Error("Timer has not been started.");
        this.enabled = false;
        this.onStop();
        clearTimeout(this.timeoutId);
    }

    restart() {
        this.stop();
        this.start();
    }

    /**
     * Internal function used to call the onTick function and schedule the next call.
     */
    _tick() {
        Promise.resolve(this.onTick()).then(() => {
            if (this.enabled) {
                this.timeoutId = setTimeout(() => this._tick(), this.interval);
            }
        });
    }
}

let t = new Timer(() => console.log('Tick'), { 
    refreshDelay: 1,
    onStart: () => console.log('Start'),
    onStop: () => console.log('Stop')
})

t.start()
setTimeout(() => t.restart(), 4000)
setTimeout(() => t.stop(), 8000)