3
\$\begingroup\$

I was using setTimeouts for a project where a callback will be called after a duration, but the user has the options of extending, pausing or stopping the timeout. I felt using the default setTimeout was a bit clunky, especially since I needed to remember the timeout ids, and I decided to create a timeout utility object. I was wondering if I'm doing this properly, and whether you could give me some tips on how to refactor my code?

Object definition

//Utility object that makes it easier to control timeouts
function Timer(duration, callback) {
    this._duration = duration;
    this._callback = callback;
    this._id = -1;

    this._paused = true;
    this._lastRunTime = -1;
    this._remainingTime = this._duration;
}

//calls the current timeout; if a function is passed, run that function
//instead of default callback (but do not change the default callback)
Timer.prototype.callTimer = function(aCallback, aDuration) {
    var that = this,
        //sets callback and duration to default values if not passed in
        callback = aCallback || this._callback,
        duration = aDuration || this._duration;

    //if a previous timeout has been set, clear it
    if (this._id > -1 ) {
        clearTimeout(this._id);
    }
    //executes the callback and resets the timer
    this._id = setTimeout(function() {
        callback();
        that.resetTimer();
    }, duration);

    this._paused = false;
    //rememebers the time when the timer was run (for pauseTimer)
    this._lastRunTime = new Date();
}

Timer.prototype.resetTimer = function() {
    this._remainingTime = this._duration;
    this._paused = true;
    this._lastRunTime = -1;
    clearTimeout(this._id);
}

//changes the default callback
//if second argument is true, execute callTimer
Timer.prototype.changeCallback = function(callback, callTimer) {
    this._callback = callback;
    if (callTimer) this.callTimer();
}

Timer.prototype.pauseTimer = function() {
    if (this._lastRunTime === -1) {
        throw new Error('Timer has not been run yet');
    }

    //if currently paused, call timer with remaining time left
    if (this._paused) {
        this._lastRunTime = new Date();
        this.callTimer(this._callback, this._remainingTime);
    } 
    //else pause
    else {
        clearTimeout(this._id);
        //subtract the amount of time that has elapsed 
        this._remainingTime -= (new Date() - this._lastRunTime);
    }

    //toggles the _paused variable
    this._paused = !this._paused;
}

http://fiddle.jshell.net/6nhGW/

Test Code

var button = document.getElementById('extendTimeout'),
    button2 = document.getElementById('tempChangeCallback'),            
    button3 = document.getElementById('changeCallback'),
    button4 = document.getElementById('resetTimer'),    
    button5 = document.getElementById('pauseTimer'),        
    startTime,
    timeout = -1;

var timer = new Timer(3000, function() {
    console.log('Total time elapsed: ' + (new Date() - startTime));
    startTime = null;    
})

button.addEventListener('click', function() {
    if (!startTime) startTime = new Date();
    timer.callTimer();
})

button2.addEventListener('click', function() {
    if (!startTime) startTime = new Date();    
    timer.callTimer(function() {
        console.log('Temporarily change callback');
        console.log('Total time elapsed: ' + (new Date() - startTime));
        startTime = null;
    });
})

button3.addEventListener('click', function() {
    timer.changeCallback(function() {
        console.log('Callback has been changed!');
        startTime = null;
    }, true);
});

button4.addEventListener('click', function() {
    timer.resetTimer();
    startTime = null;
});

button5.addEventListener('click', function() {
    if (!startTime) startTime = new Date();    
    timer.pauseTimer();
});
\$\endgroup\$
1
\$\begingroup\$

From a once over:

  • I would not use _ to prefix all your variables, it would make more sense to either actually use private variables or write a comment that this variables should not be touched by callers
  • pauseTimer lies, since you toggle. I would call it toggleTimer or startStopTimer
  • Your code is well commented except for //toggles the _paused variable which is kind of obvious ;)
  • Besides some missing semicolons, your code looks fine on JsHint.com
  • I think in callTimer you could have called your paramater callback, then do callback = callback || this.callback and then use callback in your closure.

All in all, good, re-usable code.

For my own purposes I would probably make the private variables really private and move the functions out of prototype into the constructor. That would increase memory footprint, but I would never use more than a dozen of these at the same time.

\$\endgroup\$
  • \$\begingroup\$ @konijin Thank you for your comments, and it's really helpful! I considered using private variables but decided against it due to the increased memory footprint, but you make a good point that we don't really need too many of them around. \$\endgroup\$ – Dan Tang Mar 13 '14 at 2:41
  • \$\begingroup\$ On overriding the callback in callTime, I don't fully understand what you mean - do you mean that we shouldn't do callback = callback || this.callback, and instead do this.callback = callback || this.callback? The only problem is this would overwrite the default callback that we set initially? \$\endgroup\$ – Dan Tang Mar 13 '14 at 2:44
  • \$\begingroup\$ Yes, the default would be changed, I guess if you call it 3 times with a different callback that it would not make sense to change this.callback, will update my question. \$\endgroup\$ – konijn Mar 13 '14 at 23:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.