1
\$\begingroup\$

I was using dataTables.net's responsive plugin before this, and am happy to say that my (much simpler) code runs about 5 times quicker than that did. On my machine the fiddle averages about 50ms versus 250ms if I use DT responsive.

Is there anything I should do to improve performance of my code? Can it be made better? I'm FAR from a JS expert, as I'm sure it shows. I'm very happy to drop jQuery if it can be done better without!

Here's a working JS fiddle.

function initResponsiveTables() {
    //does this enhance performance at all?
    'use strict';

    //get an array of <th> elements
    var th = $('th');

    //initialise in top scope
    var priorities = [];

    //iterate over the <th>s, extracting their column priorities
    th.each(function (index) {
        var priority = th[index].getAttribute('data-priority');

        //TODO: this seems very messy, I feel there must be a cleaner way?
        if (priorities[priority] === undefined) {
            priorities[priority] = index;
        } else {
            priorities[priority] += ',' + index;
        }
    });

    //clean up the array so we have consecutive indices
    priorities = priorities.filter(function (x) {
        return x !== undefined && x !== null;
    });

    //initialise all of these outside of the loop
    var sCols = '';
    var aCols = [];
    var aSelectors = [];
    var sSelectors = '';
    var i;

    //while the table is wider than the window, and there's still columns to remove
    while ($('#table').width() > $(window).width() - 40 && priorities.length > 0) {
        //get the lowest priority
        sCols = (priorities.pop() + '');

        //split it, in case we have more than one column on the same priority
        //in this case we prefer to remove them as a group, rather than one at a time
        aCols = sCols.split(',');

        //clear the array from previous iteration
        aSelectors = [];
        for (i = 0; i < aCols.length; i++) {
            //build up the jQuery selectors
            aSelectors.push('td:nth-child(' + aCols[i] + '), th:nth-child(' + aCols[i] + ')');
        }

        //make jQuery selector string
        sSelectors = aSelectors.join(', ');

        //hide relevant columns
        $(sSelectors).css('display', 'none');
    }
}

var start = performance.now();;

initResponsiveTables();

alert(performance.now() - start);
\$\endgroup\$
  • \$\begingroup\$ out of curiosity why are you not doing this in CSS? \$\endgroup\$ – Dan Pantry Dec 23 '15 at 11:53
  • \$\begingroup\$ Because I don't think that's possible \$\endgroup\$ – Codemonkey Dec 23 '15 at 11:55
  • \$\begingroup\$ Let us continue this discussion in chat. \$\endgroup\$ – Dan Pantry Dec 23 '15 at 12:19
  • 1
    \$\begingroup\$ As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. \$\endgroup\$ – Jamal Dec 23 '15 at 18:23
2
\$\begingroup\$

I know the people on here can do a way better job, but here it is without jQ and those array function things. Hopefully someone here will give you some real advise. I just wanted to get rid of the jQ.

One note, you don't need to array.join strings any more.

...only found that out about a year ago myself.

Your main bottle necks will be in how you alter/touch the dom, but I'm no good at those points... google I guess.

All this does give you more speed, but I bet theres people here that would hate me not using array function stuff. But you seemed to be into speed so that's what I gave you. Me I can go either way...speed this, comfort that.

function initResponsiveTables() {
  //does this enhance performance at all?
  'use strict';

  //get an array of <th> elements
  var th = document.getElementsByTagName('th');

  //initialise in top scope
  var priorities = [];

  //iterate over the <th>s, extracting their column priorities
  for (var i = 0, end = th.length; i < end; i += 1) {

    var priority = th[i].getAttribute('data-priority');

    //TODO: this seems very messy, I feel there must be a cleaner way?
    if (priorities[priority] === undefined) {
      priorities[priority] = i;
    } else {
      priorities[priority] += ',' + i;
    }
  }

  for (var spot = 0, i = 0, end = priorities.length, x; i < end; i += 1) {
    x = priorities[i];
    if (x !== undefined && x !== null) {
      priorities[spot] = x;
      spot += 1;
    }

  }
  priorities.length = spot;

  //initialise all of these outside of the loop
  var sCols = '';
  var aCols = [];
  var aSelectors = [];
  var sSelectors = '';
  var i;

  var table = document.getElementById('table');

  while (table.getBoundingClientRect().width > window.document.documentElement["clientWidth"] - 40 && priorities.length > 0) {

    //get the lowest priority
    sCols = (priorities.pop() + '');

    //split it, in case we have more than one column on the same priority
    //in this case we prefer to remove them as a group, rather than one at a time
    aCols = sCols.split(',');

    //clear the string from previous iteration
    aSelectors = '';
    for (i = 0; i < aCols.length - 1; i += 1) {
      //build up the selectors
      aSelectors += ('td:nth-child(' + aCols[i] + '), th:nth-child(' + aCols[i] + ')') + ', ';
    }
    aSelectors += ('td:nth-child(' + aCols[i] + '), th:nth-child(' + aCols[i] + ')');

    var els = table.querySelectorAll(aSelectors);
    for (var i = 0, end = els.length; i < end; i += 1) {
      els[i].style.display = 'none';
    }
  }
}

var start = performance.now();;

initResponsiveTables();

alert(performance.now() - start);

EDIT:
Couldn't sleep its like 40 degrees C here right now so thought Id make you an example to show the difference when you dont thrash the dom. I think the technical term (bad memory) would be trying to avoid reflows.

Where yours is is pretty obvious, this....
while (table.getBoundingClientRect().width > window.document.documentElement["clientWidth"] - 40 && priorities.length > 0) {
...specificly....
table.getBoundingClientRect().width for sure and maybe window.document.documentElement["clientWidth"]
...combined with...
els[i].style.display = 'none';

Your, in a loop, changing the dom and then asking for a prop that definitely causes a reflow/repaint/wotevaitscalled if the dom has changed since last call and that stuff is real costly.

So what if we knew the width of the cells and only had to get the width of the table and window once? Well youd end up with code like below and if you test it youll see it is faster, ESPECIALLY in FireFox (didnt test IE).

I pre picked the width of the cells, but you could look into maybe getting it from the already rendered table (hate that by the way) or other options, or just stick with a set width which the browser would like anyway (easier to paint).

http://plnkr.co/edit/oeJpGK?p=preview I prfr plnkr ;)

function initResponsiveTables() {
  //does this enhance performance at all?
  'use strict';

  var th = document.getElementsByTagName('th');
  var priorities = [];
  for (var end = 0, i = th.length - 1; i >= end; i -= 1) {

    var priority = th[i].getAttribute('data-priority');

    //TODO: this seems very messy, I feel there must be a cleaner way?
    if (priorities[priority] === undefined) {
      priorities[priority] = [
        i
      ];
    } else {
      priorities[priority].push(i);
    }
  }

  var removeCols = [];
  // Looking at the idea of having a set width
  // Im not great at html, depending on how tables work you might be able to get the width after the table is created instead of using a set width. worse come to worse scan each row for biggest cell
  // its also set in the css
  var colWidth = 85;

  var table = document.getElementById('table');
  var tableWidth = table.getBoundingClientRect().width;
  var targetDiff = table.getBoundingClientRect().width - window.document.documentElement["clientWidth"] - 40;
  var currentDiff = 0;
  var colIndex = priorities.length - 1;
  var cols;

  if (targetDiff > 0) {
    // var atLeast = Math.ceil(diff / colWidth); // how many cols to remove
    while (currentDiff < targetDiff && colIndex >= 0) {

      cols = priorities[colIndex];

      if (!cols) {
        //skip sparse bits
        while (cols === undefined && colIndex >= 0) {
          cols = priorities[colIndex--];
        }
      }

      if (cols) {
        cols.forEach(function(index) {
          table.tHead.rows[0].children[index].style.display = 'none';
          for (var i = 0, end = table.children[1].children.length; i < end; i += 1) {
            var row = table.children[1].children[i];
            row.children[index].style.display = 'none';
          }
        });
        currentDiff += colWidth * cols.length;
      }
      colIndex--;
    }
  }
}

var start = performance.now();;

initResponsiveTables();

alert(performance.now() - 

start);
\$\endgroup\$
  • \$\begingroup\$ To be honest, the .join (rather than concatenation) was due to what I'll call "the hanging comma problem", rather than a performance consideration. Moving the last iteration of the loop outside of the loop fixes that at the expense of repeating yourself. \$\endgroup\$ – Codemonkey Dec 23 '15 at 17:38
  • \$\begingroup\$ Thanks. Unforunately your code doesn't appear to run any faster than the jQ version, but at least it doesn't need jQ! \$\endgroup\$ – Codemonkey Dec 23 '15 at 18:05
  • \$\begingroup\$ @TheCoffeCup Thanks, thats alot neater ;P \$\endgroup\$ – PAEz Dec 24 '15 at 6:26
  • \$\begingroup\$ Perfect, thank you PAEz. I'm embarrassed that this solution didn't even /occur/ to me. I've no idea why, as it's so blindingly obvious, and I knew the multiple DOM reflows/repaints/whatever were what made the whole thing slow. A perfect solution, thank you so much! \$\endgroup\$ – Codemonkey Dec 24 '15 at 10:59
  • \$\begingroup\$ The only thing I'll change is the fixed colWidth, but it's trivial to iterate through the THs and pull their widths into an array. \$\endgroup\$ – Codemonkey Dec 24 '15 at 11:04
-2
\$\begingroup\$
  • Use pure JS (VanillaJS)
  • Use var to store tab length so it does not have to be recalculated for each iteration
  • minify your code (for page load)
  • Object Oriented JS improve performance if it's used the right way

This article shows how to write better code for the v8 engine, this is interesting.

this is not the most optimized version but I observed a gain of about 10ms (Sorry to have renamed some variables) :

function initResponsiveTables() {
    'use strict';
    var th = document.getElementsByTagName('th'),
    thL = th.length,
    table = document.getElementById('table'),
    ww = window.innerWidth - 40,
    pp = [], i, p, 
    sCols = '',
    aCols = [],
    aSelectors = [],
    sSelectors = '',
    aCL, aa, aaL, aSL;

    for (i=0; i < thL; i++) {
      p = th[i].getAttribute('data-priority');
      pp[p] =  ((!pp[p]) ? i : pp[p]+','+i);
    }
    pp = pp.filter(function (x) {
        return x !== undefined && x !== null;
    });
    while (table.offsetWidth > ww - 40 && pp.length > 0) {
        sCols = (pp.pop() + '');
        aCols = sCols.split(',');
        aSelectors = [];
        aCL = aCols.length;

        for (i = 0; i < aCL; i++)
            aSelectors.push('td:nth-child('+aCols[i]+'), th:nth-child('+aCols[i]+ ')');
                    aSL = aSelectors.length;
        for (i=0; i < aSL; i++) {
            aa = document.querySelectorAll(aSelectors[i]);
          aaL = aa.length;
          for (var j=0; j < aaL; j++)
            aa[j].style.display = 'none';
        }
    }
}

var start = performance.now();

initResponsiveTables();

alert(performance.now() - start);

Minified version :

function initResponsiveTables(){"use strict";var t,e,n,r,o,i,l=document.getElementsByTagName("th"),a=l.length,s=document.getElementById("table"),h=window.innerWidth-40,d=[],f="",c=[],u=[];for(t=0;a>t;t++)e=l[t].getAttribute("data-priority"),d[e]=d[e]?d[e]+","+t:t;for(d=d.filter(function(t){return void 0!==t&&null!==t});s.offsetWidth>h-40&&d.length>0;){for(f=d.pop()+"",c=f.split(","),u=[],n=c.length,t=0;n>t;t++)u.push("td:nth-child("+c[t]+"), th:nth-child("+c[t]+")");for(i=u.length,t=0;i>t;t++){r=document.querySelectorAll(u[t]),o=r.length;for(var p=0;o>p;p++)r[p].style.display="none"}}}var start=performance.now();initResponsiveTables(),alert(performance.now()-start);
\$\endgroup\$
  • \$\begingroup\$ don't intentionally obfuscate variable names like that (in your non-minified version). Also none of your code touches on "object orientated js" at all so I'm not sure why that was mentioned as a point to improve performance. \$\endgroup\$ – Dan Pantry Dec 23 '15 at 15:48
  • \$\begingroup\$ You're right, I didn't use OO js because i didn't want to spend more time on the question and my computer is freezing --' . But i think this is important to notice that OO improve perf and helps write better code. If someone takes the time to implement this in OO, i'd be happy to read it. \$\endgroup\$ – boehm_s Dec 23 '15 at 16:06
  • 1
    \$\begingroup\$ I'm not really sure how any of this could be implemented in OO any more or less efficiently. The bottle neck of this code is the DOM access in a while loop. \$\endgroup\$ – Dan Pantry Dec 23 '15 at 16:10
  • \$\begingroup\$ I agree Dan, thanks for your contribution though @boehm_s \$\endgroup\$ – Codemonkey Dec 23 '15 at 18:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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