Is there any way to optimise this code? [closed]

Closed. This question is opinion-based. It is not currently accepting answers.

---

开发者_JS百科

Want to improve this question? Update the question so it can be answered with facts and citations by editing this post.

Closed 9 years ago.

Improve this question

I have a piece of code which will invert all the checkboxes on my form. I have multiple elements (not just checkboxes but also <input type='text'>'s) in it. The reason I need it to be optimised is because it takes about two to three seconds to select all the checkboxes (275 right now).

Here's my code:

function FormInverse() {
    var iCheckbox = 1; // Because there are multiple input elements, we need to distinquish the input element ID and the row id
    var FormLength = document.FormFacturen.elements.length;
    for (i=0; i < FormLength; i++) {
        var FormElementType = document.FormFacturen.elements[i].type;
        if (FormElementType == "checkbox") {
            var Elements = document.getElementsByClassName('row' + iCheckbox); // Alle elementen in de array zetten
            var iNumElements = Elements.length;
            for (iElement=0; iElement < iNumElements; iElement++) {
                if (document.FormFacturen[i].checked == true) {
                    Elements[iElement].className = "invoice-tr-standard row" + iCheckbox;
                } else {
                    Elements[iElement].className = "invoice-tr-clicked row" + iCheckbox;
                }
            }
            iCheckbox++;
            document.FormFacturen[i].checked = !document.FormFacturen[i].checked;
        }
    }   
}

And here is the document.getElementsByClassName function:

document.getElementsByClassName = function(cl) {
    var retnode = [];
    var myclass = new RegExp('\\b'+cl+'\\b');
    var elem = document.getElementsByTagName('*');
    for (var i = 0; i < elem.length; i++) {
        var classes = elem[i].className;
        if (myclass.test(classes)) retnode.push(elem[i]);
    }
    return retnode;
};

---

I would suggest using jQuery as well.

Try this:

Add a reference to jQuery:

<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.6.2/jquery.min.js" type="text/javascript" language="JavaScript"></script>

Use this code:

$(':checkbox').each( function() {
  $(this).attr('checked', !$(this).attr('checked'));
});

Edited: Or use this to change the classes as well:

$(':checkbox').each(function() {
    var checked = $(this).attr('checked');
    if (checked) {
        $(this).attr('checked', false);
        $(this).addClass('invoice-tr-clicked');
        $(this).removeClass('invoice-tr-standard');        
    }
    else {
        $(this).attr('checked', true);
        $(this).addClass('invoice-tr-standard');
        $(this).removeClass('invoice-tr-clicked');
    }
});

Final version:

$('#FormFacturen :checkbox').each(function() {
    var checked = $(this).attr('checked');
    if (checked) {
        $(this).attr('checked', false);
        $(this).parents('tr').addClass('invoice-tr-clicked');
        $(this).parents('tr').removeClass('invoice-tr-standard');        
    }
    else {
        $(this).attr('checked', true);
        $(this).parents('tr').addClass('invoice-tr-standard');
        $(this).parents('tr').removeClass('invoice-tr-clicked');
    }
});

---

Each call to getElementsByClassName is expensive, and it is being called on each pass of your for loop.

In addition to @Geoff's suggestion, you could call document.getElementsByTagName('input'); just once, instead of each time getElementsByClassName is called and cache the result for use within your loop.

That would require making a small modification to your getElementsByClassName function whereby it accepts an array of elements to search through.

document.getElementsByClassName = function(cl, eles) {
    var retnode = [];
    var myclass = new RegExp('\\b'+cl+'\\b');
    var len = eles.length;
    for (var i = 0; i < len; i++) {
        var classes = eles[i].className;
        if (myclass.test(classes)) retnode.push(eles[i]);
    }
    return retnode;
};


function FormInverse() {
    // cache all inputs
    var inputs = document.getElementsByTagName("input");
    ...
    // later
    var Elements = document.getElementsByClassName('row' + iCheckbox, inputs); 

---

You should look into a library like JQuery. It will handle this kind of thing well.

There are a lot of little things you can do to improve your code though. First thing I notice is that your getElementsByClassName function is looping through ALL elements on your page every time you call it. You could change this line:

var elem = document.getElementsByTagName('*');

to just get the input elements:

var elem = document.getElementsByTagName('input');

---

I am pretty much sure the bottleneck here is the getElementsByClassName function, the browser needs to re-scan the whole html each time to find your element. I suggest you give your elements a unique id id="row1", id="row2", ... instead of a unique class name and use getElementById which is much faster.

 var Elements = document.getElementsById('row' + iCheckbox);