3
\$\begingroup\$

I have the following function that takes JSON and turns it into table rows:

function addDataFromJson(json) {
    var data = json.data;
    var rows = '';
    $('.uiGridContent tbody').html('');

    for (var i = 0; i < data.length; i++) // for each row
    {
        var columns = '';

        for (var b = 0; b < Object.keys(data[i]).length; b++) // for each column
        {
            var content = data[i][Object.keys(data[i])[b]];

            columns += '<td>' + content + '</td>';
        }

        var row = columns;

        row = '<tr>\
                    <td><input type="checkbox"></td>\
                    <td>\
                        <div class="dropdown">\
                            <a class="buildMenu" data-toggle="dropdown"><span class="arrow"><span class="icon action small">Actions</span></span></a>\
                        </div>\
                    </td>\
                    ' + columns + '\
                </tr>';

        rows =+ row;

    }

    $(rows).appendTo('.uiGridContent tbody');
}

The JSON looks like this (for those curious):

"data": [{
        "id": 1,
        "name": "<a href=\"#\">Sheffield University</a>",
        "departments": ["Software", "Recruitment", "Consulting"],
        "locations": ["Sheffield", "Rotherham", "London", "New York"]
    }, {
        "id": 2,
        "name": "<a href=\"#\">Sheffield College</a>",
        "departments": "",
        "locations": ["Hillsborough", "City Centre", "Crystal Peaks"]
    }, {
        "id": 3,
        "name": "<a href=\"#\">Sheffield High School</a>",
        "departments": ["Medical", "Family", "Criminal"],
        "locations": ["Sheffield", "Rotherham"]
    }...

However if I pass back 1000+ records then the JavaScript struggles and it takes a few seconds to do it and causes the browser to lock up.

Any ideas on how to improve this code?

It seems to be the line where I append the rows to the page:

$(rows).appendTo('.uiGridContent tbody');

As if I comment that line out, then it's really fast...

\$\endgroup\$
2
\$\begingroup\$

Fun question,

there is really nothing much you can do about this, adding a thousand rows in an HTML table will take a few second and might lock your browser. Browser were not meant for that. Moore's law is on your side though, so at it'll be fine at some point. Consider implementing paging ( with say 100 items per page ), and you app will be much snappier.

Other than that;

  • rows =+ row; <- This drove me nuts, it is rows += row;, your code does not work
  • HTML in your JavaScript, say it aint so. You could use either true HTML 5 templates or write your own <span> as a template
  • You are retrieving Object.keys far too many times, I would suggest you get it ones for the first row and cache it
  • It is considered better practice to have all your var's in ` place, and chain them

This is my counter proposal:

var data = { "data": [{
        "id": 1,
        "name": "<a href=\"#\">Sheffield University</a>",
        "departments": ["Software", "Recruitment", "Consulting"],
        "locations": ["Sheffield", "Rotherham", "London", "New York"]
    }, {
        "id": 2,
        "name": "<a href=\"#\">Sheffield College</a>",
        "departments": "",
        "locations": ["Hillsborough", "City Centre", "Crystal Peaks"]
    }, {
        "id": 3,
        "name": "<a href=\"#\">Sheffield High School</a>",
        "departments": ["Medical", "Family", "Criminal"],
        "locations": ["Sheffield", "Rotherham"]
    }]};

addDataFromJson( data );


function addDataFromJson(json) {
    var template = $('#mytemplate').html(),
        data = json.data,
        keys = Object.keys( data[0] ),
        rows = '',
        cells;
  
    $('body').html('');
  
    for (i = 0; i < data.length; i++) // for each row
    {
        cells =  keys.map( function(key){ return data[i][key]; } ).join('</td>\n<td>');
        rows += template.replace( 'DataFromJson' , cells );
    }

    $(rows.trim()).appendTo('body');
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.9.1/jquery.min.js"></script>
<template id="mytemplate" style="display:none">  
<tr>
  <td><input type="checkbox"></td>
  <td>
    <div class="dropdown">
      <a class="buildMenu" data-toggle="dropdown">
        <span class="arrow"><span class="icon action small">
          Actions
        </span></span>
      </a>
    </div>
  </td>
  <td>DataFromJson</td>
</tr>
</template> 

\$\endgroup\$
  • \$\begingroup\$ TIL about true HTML5 templates. Too bad IE doesn't support them. Guess I'll stick to cloning hidden elements using jQuery for another while \$\endgroup\$ – Johan Dec 13 '14 at 2:34
1
\$\begingroup\$

Ok, let's first go through your code

$('.uiGridContent tbody').html('');

This is bad because this doesn't remove handlers and other objects bound to the elements. It's not good housekeeping per se (Dunno if the GC will pick them up or not). It's better to use empty() or remove() as it does housekeeping for you.

for (var i = 0; i < data.length; i++)

data is an array right? Take advantage of the newer array APIs like map and forEach.

var row = columns;
...
columns += '<td>' + content + '</td>';

Not to be a bully, but this isn't really the "best" way to do this. Suggesting you use fragments if you want to programmatically create elements, or use a template engine like Mustache where you can interpolate data immediately to the template.

{
    "id": 1,
    "name": "<a href=\"#\">Sheffield University</a>",
    "departments": ["Software", "Recruitment", "Consulting"],
    "locations": ["Sheffield", "Rotherham", "London", "New York"]
}

Also, it's not safe to ship HTML in data. Aside from being a sight for sore eyes, it opens up an avenue for XSS. This is one reason why templating systems like Mustache escape data by default to avoid XSS.

As an additional tip, use Chrome's/Firefox's CPU profiler in the dev tools. Record, then run your script, then stop recording. Best if viewed using Tree View, you can see the percentage of the time it took the engine to run your script. That way, you see the hotspots for optimization.

\$\endgroup\$
  • \$\begingroup\$ The thing is, the code up until the append happens in less than half a second, so the for loop and the other parts all seem very nippy. The problem seems to lie with actually painting it into the page (like the size of the DOM). \$\endgroup\$ – Cameron Dec 13 '14 at 15:48

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.