Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I built a custom HTML5 video control bar, and for each second, it updates two readings:

  1. Time Elapsed

  2. Time Remaining

in hh:mm:ss notation, like so:

enter image description here

Because of how often this function runs, I want it to be as efficient as possible.

I created this JS Fiddle to test benchmarks. It runs the timer normally, until you un-comment the benchmark and comment the casual running func. The commented function call runs this function 1,000,000 times and logs the speed via the console's timing utility.

I'm looking for best practices, efficiency, and general improvements.


Here's the code. For testing, it's been modified to run in a loop or at a set interval rather than being connected to a video.

/* Video Time Processing Benchmark Snippet */

var timeProgress = $("#time-progress"),
    timeRemaining = $("#time-remaining");

// Set mock video duration for testing
var duration = 5000;

// Convert numbers to double+ digit format (09, 99, 999)
function digitize(n) {
    return n > 9 ? "" + n : "0" + n;
}

// Convert seconds to hh:mm:ss
function secondsToHMS(seconds) {
    var numhours = digitize(Math.floor(((seconds % 31536000) % 86400) / 3600));
    var numminutes = digitize(Math.floor((((seconds % 31536000) % 86400) % 3600) / 60));
    var numseconds = digitize(Math.floor((((seconds % 31536000) % 86400) % 3600) % 60));
    return numhours + ":" + numminutes + ":" + numseconds;
}

// Process the elapsed and remaining times to two hh:mm:ss format
function processTime(currentTime) {
    var value = (100 / duration) * currentTime;
    var progressHMS = secondsToHMS(currentTime);
    var remainingHMS = "-" + secondsToHMS(duration - currentTime);
    timeProgress.html(progressHMS);
    timeRemaining.html(remainingHMS);
}

// Test time via interval or loop
function testTime(type, number) {
    if (type == "interval") {

        // If a number isn't provided, the default is every second (1000)
        if (!number) var number = 1000;
        var i = 0;

        // Set interval to run function
        setInterval(function(){
            currentTime = i;
            processTime(currentTime);
            i++;

            // Clear the interval when duration is reached
            if (i > duration) clearInterval(Int);
        }, number);
    }
    else if (type == "loop"){
        for (var i = 0; i < duration; i++){
            currentTime = i;
            processTime(currentTime);
        }
    }
}

// Use Int to set time for clearInterval() targetng
var Int = testTime("interval");

//Get benchamark on functon, vs optimized versions
/*
console.time("Benchmark");
    testTime("loop", 1000000);
console.timeEnd("Benchmark");
*/
share|improve this question
1  
Don't use setInterval to animate, use requestAnimationFrame. This allows the browser to decide when to reanimate and sync it with general repainting. –  Boris the Spider Jul 31 at 6:20

3 Answers 3

up vote 13 down vote accepted

If you're worried about performance, this should raise a red flag:

someJQueryObject = $("#some-id");
...
someJQueryObject.html(aPlainOldString);

.html(foo) is almost certainly slower than .innerHTML = foo, because .html() does a bunch of extra stuff, like checking if it can use innerHTML, trying to use innerHTML, catching errors if that fails and using a fallback, etc. Also, using innerHTML is probably slower than simply creating a Text node and updating its data property.

I'd suggest replacing timeProgress = $("#time-progress") with something like this:

var timeProgress = document.createTextNode('');

document.getElementById('time-progress').appendChild(timeProgress);

And replacing timeProgress.html(progressHMS); with this:

timeProgress.data = progressHMS;

This will be a significant performance improvement and will also eliminate the jQuery dependency. Using a large GP library like jQuery to select two elements and update their text content seems like overkill anyway.

Here's a jsPerf demonstrating the performance difference.

share|improve this answer
    
jsPerf. Wow, this is way faster! I had to use document.createTextNode(""); though. –  Schism Jul 30 at 20:43
    
@Schism good catch, updated with the empty string. Interesting perf. :) –  Dagg Jul 30 at 20:45
    
Whoooops, the property is data, not value. No wonder it went so fast. I just perfed it and it's still way faster though. –  Dagg Jul 30 at 21:11
    
I have to admit, while the other answers are valuable, this one mostly clearly demonstrates a performance increase. +1 and check. –  jt0dd Jul 31 at 6:03
2  
@Falco I'd love to but I can't; comments can only be edited in the first five minutes. I've edited it into the question. –  Schism Jul 31 at 13:41

When converting time values like this I find it much simpler, and more readable to work up from the smallest units. Consider the following:

// Convert seconds to hh:mm:ss
function secondsToHMS(intime) {
    var numseconds = digitize(intime % 60);
    intime = Math.floor(intime / 60);
    var numminutes = digitize(intime % 60);
    intime = Math.floor(intime / 60);
    var numhours = digitize(intime);
    return numhours + ":" + numminutes + ":" + numseconds;
}

The intime variable is modified as the time runs through. It first counts seconds, then minutes, then hours.

as for the performance, I imagine it is faster because the calculations are fewer, and simpler, but I have not benchmarked it. Even if it is not faster, I would still prefer it.

share|improve this answer
    
jsPerf. About the same speed, but this is more readable. –  Schism Jul 30 at 20:42
    
Thanks @Schism - on my machine (Firefox), my solution came out ahead by 15% –  rolfl Jul 30 at 21:01
    
+1 for readability x10 –  jt0dd Jul 31 at 6:01
  • I (like many others) prefix my jQuery object variables with a $ to make $timeProgress and $timeRemaining. To me, this is a nice explicit reminder that they're jQuery objects, à la Hungarian notation.

  • duration seems to me like it could be better named totalTime or something.

  • In digitize(), I see little point in doing "" + n. Use return (n > 9 ? "" : "0") + n or return n > 9 ? n : "0" + n.

  • I know that you stripped out your code, but here, you don't use value in processTime().

  • In testTime(), you aren't actually passing through the return value of setInterval, so you won't be able to go and clearInterval with Int (which, incidentally, should be lowercased renamed to something like testIntervalHandle or something).

  • Not that it'll matter in production/release, but you're polluting global namespace with currentTime in testTime().

Finally, I don't imagine performance is as crucial as you make it out to be; once per second is pretty lenient.

share|improve this answer
1  
+1 for pointing out that performance may not be a issue at all. This is basically an animation set to run at 1 fps - that should be doable :) –  Flambino Jul 31 at 1:56
1  
+1 for formatting, naming, and namespace, but most of all because I didn't realize that once per second mattered so little. However, perhaps learning the most efficient method in situations like this, where it hardly matters, will prepare me (and maybe even you?) for a situation where a similar thing needs to be done much faster than this. –  jt0dd Jul 31 at 6:06

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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