1

I have a game I'm creating where lights run around the outside of a circle, and you must try and stop the light on the same spot three times in a row. Currently, I'm using the following code to loop through the lights and turn them "on" and "off":

var num_lights = 20;
var loop_speed = 55;
var light_index = 0;
var prevent_stop = false; //If true, prevents user from stopping light

var loop = setTimeout(startLoop, loop_speed);

function startLoop() {
    prevent_stop = false;
    $(".light:eq(" + light_index + ")").css("background-color", "#fff");
    light_index++;
    if(light_index >= num_lights) {
        light_index = 0;
    }
    $(".light:eq(" + light_index + ")").css("background-color", "red");
    loop = setTimeout(startLoop, loop_speed);
}

function stopLoop() {
    clearTimeout(loop);
}

For the most part, the code seems to run pretty well, but if I have a video running simultaneously in another tab, the turning on and off of the lights seems to chug a bit. Any input on how I could possibly speed this up would be great.

For an example of the code from above, check out this page: http://ericditmer.com/wheel

5 Answers5

2

When optimizing the thing to look at first is not doing twice anything you only need to do once. Looking up an element from the DOM can be expensive and you definitely know which elements you want, so why not pre-fetch all of them and void doing that multiple times?

What I mean is that you should

var lights = $('.light');

So that you can later just say

 lights.eq(light_index).css("background-color", "red");

Just be sure to do the first thing in a place which keeps lights in scope for the second.

EDIT: Updated per comment.

sorpigal
  • 25,504
  • 8
  • 57
  • 75
2

I would make a global array of your selector references, so they selector doesn't have to be executed every time the function is called. I would also consider swapping class names, rather than attributes.

Here's some information of jQuery performance:

http://www.componenthouse.com/article-19

EDIT: that article id quite old though and jQuery has evolved a lot since. This is more recent: http://blog.dynatrace.com/2009/11/09/101-on-jquery-selector-performance/

Diodeus - James MacFarlane
  • 112,730
  • 33
  • 157
  • 176
1

One thing I will note is that you have used a setTimeout() and really just engineered it to behave like setInterval().

Try using setInterval() instead. I'm no js engine guru but I would like to think the constant reuse of setTimeout has to have some effect on performance that would not be present using setInterval() (which you only need to set once).

Edit:

Curtousy of Diodeus, a related post to back my statement:

Related Stack Question - setTimeout() vs setInterval()

Matthew Cox
  • 13,566
  • 9
  • 54
  • 72
1

You could try storing the light elements in an array instead of using a selector each time. Class selectors can be a little slow.

var elements = $('.light');

function startLoop() {
    prevent_stop = false;
    $(elements[light_index]).css('background-color', '#fff');
    ...
}

This assumes that the elements are already in their intended order in the DOM.

Joe Friedl
  • 751
  • 3
  • 10
0

OK, this includes some "best practice" improvements, if it really optimizes the execution speed should be tested. At least you can proclaim you're now coding ninja style lol

// create a helper function that lend the array reverse function to reverse the 
// order of a jquery sets. It's an object by default, not an array, so using it
// directly would fail
$.fn.reverse = Array.prototype.reverse;
var loop,
    loop_speed = 55,
    prevent_stop = false,
    // prefetch a jquery set of all lights and reverses it to keep the right
    // order when iterating backwards (small performance optimization)
    lights = $('.light').reverse();


// this named function executes as soon as it's initialized
// I wrapped everything into a second function, so the variable prevent_stop is
// only set once at the beginning of the loop
(function startLoop() {
    // keep variables always in the scope they are needed
    // changed the iteration to count down, because checking for 0 is faster.
    var num_lights = light_index = lights.length - 1;
    prevent_stop = false;
    // This is an auto-executing, self-referencing function
    // which avoids the 55ms delay when starting the loop
    loop = setInterval((function() {
        // work with css-class changing rather than css manipulation
        lights.eq( light_index ).removeClass('active');
        // if not 0 iterate else set to num_lights
        light_index = (light_index)? --light_index:num_lights;
        lights.eq( light_index ).addClass('active');
        // returns a referenze to this function so it can be executed by setInterval()
        return arguments.callee;
    })(), loop_speed);
})();


function stopLoop() {
    clearInterval(loop);
}

Cheers neutronenstern

Marcello di Simone
  • 1,612
  • 1
  • 14
  • 29