HiveBrain v1.2.0
Get Started
← Back to all entries
snippetjavascriptMinor

How can I improve performance of my selecting code

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
canimproveperformanceselectinghowcode

Problem

I have a page with a list of items. Items have several actions assigned to them. (see screenshot).

One may choose to directly click on an icon next to each row or check a checkbox on the left hand side and click action above the list.

The issue is that after clicking an item OR checking a checkbox of several items and then clicking an action there is a lag (a second or so). Imagine having 100 rows or more.



Script:

$("a.action:not(.remove)").click(function (e) { // .remove => do not execute on download tasks page
            var selected = new Array(); // array to hold selections or clicks
            var obj = $(this), objData = obj.data();
            e.preventDefault();
            if (!obj.hasClass('disablelink')) {                
                if (objData.ajaxMachineid) {
                    selected.push(objData.ajaxMachineid);
                } else {
                    $("input.checkbox:checkbox:checked:not(.checkall)").each(function () {
                        var checkbox = $(this), machineId = checkbox.val(), packageId = objData.ajaxPackageid.removeSpecialChars().toUpperCase(), operation = objData.ajaxType;
                        var $row = $("#" + machineId + packageId.removeSpecialChars().toUpperCase());
                        if ($row.length) {
                            $row.has("a[data-ajax-type=" + operation + "]:not(.hide)").length ? selected.push(machineId) : $(this).attr('checked', false);
                        }
                    });
                }
                if (selected.length > 0) {
                    // do something more here blah blah blah
                };
            }
            $("input.checkall").attr("checked", false);
        });


Sample HTML of one row:


GD009000246
PCGames
Up

10.48.1.236   
   
   View
Install
   Remove                              
       

Solution

So pretty much everything will make a insignificant effect apart from the section where you are looping through each of the checkboxes. See my modifications and notes below.

$("a.action:not(.remove)").click(function (e) { // .remove => do not execute on download tasks page
     var selected = new Array(); // array to hold selections or clicks
     var obj = $(this),
         objData = obj.data(),

         // Moved these up here as they are not specific to the checkboxes
         packageId = objData.ajaxPackageid.removeSpecialChars().toUpperCase(),
         operation = objData.ajaxType;

     e.preventDefault();
     if (!obj.hasClass('disablelink')) {
         if (objData.ajaxMachineid) {
             selected.push(objData.ajaxMachineid);
         } else {

             // This seems a little convoluted to me, normally you would 
             // use [type=checkbox]
             $("input[type=checkbox]:checked:not(.checkall)").each(function () {

                 var checkbox = $(this),
                     machineId = checkbox.val();

                 // No need to call .removeSpecialChars().toUpperCase() on packageId
                 var $row = $("#" + machineId + packageId);

                 if ($row.length) {

                     // Not 100% whether you need .length here
                     // It could also be beneficial to add a class to the row instead
                     // of calling this complex selector
                     $row.has("a[data-ajax-type=" + operation + "]:not(.hide)").length 
                         ? selected.push(machineId) 
                         : $(this).attr('checked', false);

                 }
             });
         }
         if (selected.length > 0) {
             // do something more here blah blah blah
         };
     }
     $("input.checkall").attr("checked", false);
 });


The obvious thing to do on top of my suggestions above is to cache the checkbox list, this means that every time it enters that section, the app will no longer need to go through every element in the DOM looking for all the checkboxes. Doing this will probably have the most meaningful impact on performance.

// in document ready
var checkboxes = $('input[type=checkbox]');

// When you need to find the checked check boxes
var checked = checkboxes.find(':checked:not(.checkall)');

Code Snippets

$("a.action:not(.remove)").click(function (e) { // .remove => do not execute on download tasks page
     var selected = new Array(); // array to hold selections or clicks
     var obj = $(this),
         objData = obj.data(),

         // Moved these up here as they are not specific to the checkboxes
         packageId = objData.ajaxPackageid.removeSpecialChars().toUpperCase(),
         operation = objData.ajaxType;

     e.preventDefault();
     if (!obj.hasClass('disablelink')) {
         if (objData.ajaxMachineid) {
             selected.push(objData.ajaxMachineid);
         } else {

             // This seems a little convoluted to me, normally you would 
             // use [type=checkbox]
             $("input[type=checkbox]:checked:not(.checkall)").each(function () {

                 var checkbox = $(this),
                     machineId = checkbox.val();

                 // No need to call .removeSpecialChars().toUpperCase() on packageId
                 var $row = $("#" + machineId + packageId);

                 if ($row.length) {

                     // Not 100% whether you need .length here
                     // It could also be beneficial to add a class to the row instead
                     // of calling this complex selector
                     $row.has("a[data-ajax-type=" + operation + "]:not(.hide)").length 
                         ? selected.push(machineId) 
                         : $(this).attr('checked', false);

                 }
             });
         }
         if (selected.length > 0) {
             // do something more here blah blah blah
         };
     }
     $("input.checkall").attr("checked", false);
 });
// in document ready
var checkboxes = $('input[type=checkbox]');

// When you need to find the checked check boxes
var checked = checkboxes.find(':checked:not(.checkall)');

Context

StackExchange Code Review Q#23524, answer score: 2

Revisions (0)

No revisions yet.