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.

This is a script I use to count how many items are selected in a form. Currently it loops through the entire form to count how many check boxes are selected, each time a checkbox is clicked. The form has thousands of checkboxes, and it's painfully obvious how slow the script is with this many elements (About 18,240 items in my sample query). Any ideas on how I can speed this up?

The speed is fine when there's less than 1,000 results, but there's rarely that few when it's running in production.

function countSelected()
{
    var daform  = document.forms.resultsForm;
    var daspan  = document.getElementById("acctSelected");
    var counter = 0;
    var i       = 0;

    if (daform.multi.length == undefined) {
        if (daform.multi.checked) {
            counter = "1";
        } else {
            counter = "0";
        }
    } else {
        for (i = 0; i < daform.multi.length; i++)
        {
            if (daform.multi[i].checked) {
                counter++;
            }
        }
    }

    daspan.innerHTML = counter;
}

Fun Fact:

  • This script is fastest in Firefox 16.0.2 (about 2 secs)
  • Second fastest by a slim margin in Internet Explorer 9 (about 2.25 secs)
  • And absurdly slow in Chrome Version 23.--- (I got tired of waiting)

This is the solution I came up with based on the selected answer:

// To call the function from each checkbox...
onClick="countSelected(this.checked);"


// The function...
var counter = 0;
function countSelected(checked)
{
    var daspan  = document.getElementById("acctSelected");

    if (checked) {
        counter++;
    } else {
        counter--;
    }

    daspan.innerHTML = counter;
}
share|improve this question
    
Adding a listener to every checkbox is pretty inefficient (though much better than your original). See my answer for how to use a single listener on the form instead (same concept, different implementation). –  RobG Nov 9 '12 at 0:44
1  
@jdstankosky I would use the onchange event instead since there might be other ways for the user to check the checkbox other than clicking (i.e. keyboard). –  crdx Nov 9 '12 at 8:14

4 Answers 4

up vote 4 down vote accepted

I suspect you're going to struggle to optimise that, as DOM manipulation is always slow. See this guy's answer.

You could try do something like he suggests, keeping your data and the DOM separate from each other.

Currently it loops through the entire form to count how many check boxes are selected, each time a checkbox is clicked

Why not increment or decrement a counter when a checkbox's state is changed? That'll be a lot quicker.

share|improve this answer
    
I thought of that, but I wasn't sure if a javascript variable will persist through multiple function calls? –  jdstankosky Nov 8 '12 at 16:47
    
Declare the variable in the global scope (outside of any functions) with var whatever;, and then it will persist for the lifetime of the page. –  crdx Nov 8 '12 at 16:52
    
That helped me out tremendously, thank you. I've updated my question to include a working answer based on the ++ and --. –  jdstankosky Nov 8 '12 at 17:29
    
DOM access is not slow. DOM manipulation is. (really read that guy's answer) And you are doing it once. –  SPee Nov 8 '12 at 18:53
    
@SPee isn't it still relatively slow, as we can see by how long it takes to read the state of so many checkboxes? –  crdx Nov 8 '12 at 23:09

It sounds like you have your answer, but I wanted to contribute another point of view, maybe just for later reference or the good of the people :-)

Something that would most definitely speed things up would be to hold references to the objects instead of fetching them from the DOM every time. A simple example would be:

Instead of:

function doSomethingTenMillionTimes(){
    var awesomeButton = document.getElementById("awesomeButtonId");
    //...do something with it
}

Do this:

var awesomeButton = null;

function doSomethingTenMillionTimes(){
    getAwesomeButton()...
    //...do something with it
}

function getAwesomeButton(){
    if(awesomeButton === null){
        awesomeButton = document.getElementById("awesomeButtonId");
    }
    return awesomeButton;
}

If you have to do this for a large number of objects, once you have them retrieved, say in an array, just save that array to a variable and by storing a reference to it instead of having the DOM deliver it up each time, it becomes instantly available.

I had to do something like this recently, and solved it by storing an array of the objects I needed, assigning each of the objects a unique identifier (object.rolodexIndex = x), and when one of those objects got clicked on, I could pull it or any of it's accompanying objects out of the array rapidly and do what I needed with them.

Some other food for thought regarding large JavaScript operations:

When you are doing massive operations in JavaScript, it ties up the entire page. The solution is to insert breaks in between operations to allow for breathing room. This gives the illusion of multitasking and doesn't lock other things up. There's an excellent breakdown of how to do this here:

http://www.sitepoint.com/multi-threading-javascript/

share|improve this answer

Maybe the slowlyness is because you do not use the default functions. I never heard of the function "multi".

function countSelected()
{
    var form  = document.forms.resultsForm;
    var counter = 0;

    for (var i = 0; i < form.elements.length; i++)
    {
        var formField = form.elements[i];
        if (formField.type == "checkbox" && formField.checked) {
            counter++;
        }
    }
    document.getElementById("acctSelected").innerHTML = counter;
}

You can loop through all the elements in the form and (optionally) check if the type is a checkbox.

share|improve this answer
2  
multi is the name on the checkboxes. –  jdstankosky Nov 8 '12 at 16:23
    
Ah. With that you are basically performing a Named lookup for/with every record. This is not the fastest method to access elements in the page. –  SPee Nov 8 '12 at 18:56
    
What would be faster? –  jdstankosky Nov 8 '12 at 19:18

In your code:

> if (daform.multi.length == undefined) {

if multi is the name of all the checkboxes, then dataform.multi will return either:

  1. one element if there's only one form control named multi
  2. an HTML collection of all the form controls named multi if there's more than one
  3. undefined if there are no elements with a name of multi (and no form properties of that name).

So the only case that the above test will "work" is in #2. In #3 it will throw an error.

Much better to do:

var multis = daform.multi; // might be a collection, DOM element or undefined

if (typeof multis != 'undefined') { // might be a collection or DOM element

  if (multis.tagName) {
    // dealing with an element

  } else {
    // dealing with a collection
  }

} else {
  // there are no form controls named multi
}

Anyhow, consider instead putting a single click listener on the form. If it gets a click from a checkbox named 'multi', and it's checked, add one to a "checked" varable. If it's not checked, subract one. e.g.

<form name="daform" onclick="countCheckedMultis(event);">
  <input type="checkbox" name="multi" value="..."> 
  <input type="checkbox" name="multi" value="..."> 
  <input type="checkbox" name="multi" value="..."> 
</form>

and the function:

var countCheckedMultis = (function() {

  // Keep running total in a closure ("private" member)
  var numberChecked = 0;

  return function (evt) {
    var el = evt.target || evt.src;

    // Deal with case where target isn't an element node      
    if (el.nodeType != 1) el = el.parentNode;

    // Increment or decrement counter if came from multi element
    // depending on if it's checked or not
    if (el.name == 'multi') numberChecked += el.checked? 1 : -1;

    // Debug
    alert(numberChecked);
  }
}());

The flaw in the above is that if the page is refreshed, some browser will keep all the checked checkboxes checked, but reset the counter. So you need to initialise the counter on the first click.

Note that the collection returned by daform.multi is converted to an array before processing. This hugely increases performance, looping over 10,000 checkboxes takes less than a second in Firefox 15, Chrome 22 and IE 9 on a modest PC.

// Convert obj to array (simple function for this case)
function toArray(obj) {
  var result = [];
  for (var i=0, iLen=obj.length; i<iLen; i++) {
    result[i] = obj[i]
  }
  return result;
}

var countCheckedMultis = (function() {
  var initialised;
  var numberChecked = 0;

  return function (evt) {
    var el = evt.target || evt.src;

    if (el.nodeType != 1) el = el.parentNode;

    // If this is the first run, need to count checked checkboxes
    // as page may have been reloaded, resetting the counter but not
    // the checked checkboxes in some browsers  
    if (!initialised && el.form) {
      var multis = el.form.multi;

      // Converting a collection to an array before processing
      // hugely increases speed
      if (multis && !multis.tagName) {
        multis = toArray(multis);

        for (var i=0, iLen=multis.length; i<iLen; i++) {
          if (multis[i].checked) ++numberChecked;
        }
      }
      initialised = true;
    } else {
      if (el.name == 'multi') numberChecked += el.checked? 1 : -1;
    }

    console.log(numberChecked);
  }
}());
share|improve this answer
    
Take note of var multis = el.form.multi; and setting length to a variable in for (var i=0, iLen=multis.length; i<iLen; i++). This might not look like much and newer browsers might already optimize it away, but in general case these expressions would be evaluated on every loop iteration. Preassigning them to variables like RobG did should save some time on older browsers at least. –  scrwtp Nov 18 '12 at 10:36

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.