3

i am having problems with the below code:

function showTableData()
{
    var tableArray;
    var x = 0;
    var theHTML;

    for (i = 0; i < 7032; i++)
    {
        if (x = 0)
        {
            theHTML = '<tr>' + 
                '<th scope="row" class="spec">' + partNum[i] + '</th>' + 
                '<td>' + Msrp[i] + '</td>' + 
                '<td>' + blah[i] + '</td>' + 
                '<td>' + blahs[i] + '</td>' + 
            '</tr>' + theHTML;
            x++;
        }else{
            theHTML = '<tr>' + 
                '<th scope="row" class="specalt">' + partNum[i] + '</th>' + 
                '<td class="alt">' + Msrp[i] + '</td>' + 
                '<td class="alt">' + blah[i] + '</td>' + 
                '<td class="alt">' + blahs[i] + '</td>' + 
            '</tr>' + theHTML;
            x--;
        }
    }
    theHTML = '<table id="mytable" cellspacing="0">' + 
    '<tr>' + 
        '<th scope="col" abbr="Configurations" class="nobg">Part Number</th>' + 
        '<th scope="col" abbr="Dual 1.8">Msrp Price</th>' + 
        '<th scope="col" abbr="Dual 2">blahs Price</th>' + 
    '<th scope="col" abbr="Dual 2.5">Low Price</th>' + 
    '</tr>' + theHTML + '</table>';

    $('#example').append(theHTML);
}
 </script>

 <div id="example">
 </div>

The problem being that the $('#example').append(theHTML); never executes (or shows on the page). I think its because the string is soooooo long! It has over 7,000 items in the array so im not sure if thats the reason or if its something else?

Any help would be great! Thanks!

David

Steven de Salas
  • 20,944
  • 9
  • 74
  • 82
StealthRT
  • 10,108
  • 40
  • 183
  • 342

5 Answers5

11

Apart from the if (x = 0) that should really be if (i % 2 === 0), you really should improve performance here by using Array.join() method instead of concatenating strings. This will have a similar effect to a StringBuilder in C# or Java.

For example:

function showTableData()
{
    var tableArray;
    var theHTML = [];
    theHTML.push('<table id="mytable" cellspacing="0">',
    '<tr>', 
        '<th scope="col" abbr="Configurations" class="nobg">Part Number</th>', 
        '<th scope="col" abbr="Dual 1.8">Msrp Price</th>',
        '<th scope="col" abbr="Dual 2">blahs Price</th>', 
    '<th scope="col" abbr="Dual 2.5">Low Price</th>', 
    '</tr>');

    for (var i = 0; i < 7032; i++)
    {
        if (i % 2 == 0)
        {
             theHTML.push('<tr>', 
                 '<th scope="row" class="spec">', partNum[i], '</th>',
                 '<td>', Msrp[i], '</td>', 
                 '<td>', blah[i], '</td>', 
                 '<td>', blahs[i], '</td>', 
             '</tr>');
        } else {
             theHTML.push('<tr>',
                 '<th scope="row" class="specalt">', partNum[i], '</th>', 
                 '<td class="alt">', Msrp[i], '</td>', 
                 '<td class="alt">', blah[i], '</td>', 
                 '<td class="alt">', blahs[i], '</td>',
             '</tr>');
        }
    }
    theHTML.push('</table>');

    $('#example').append(theHTML.join(''));
}
 </script>

 <div id="example">
 </div>

The reason why appending string 'my' + ' appended' + ' string' is slower than joining strings ['my', ' joined', ' string'].join('') is because JavaScript strings are immutable so in the former example there is a third string created every time 2 strings are concatenated, which is a very expensive operation compared to adding new entries into an array.

See also a Javascript StringBuilder project built using the same priciples of Array.push() and Array.join().

The performance improvement on this project for 10,000 concatenations in IE was down from over 1 minute to 0.23 seconds.

UPDATE: Additional calls to Array.join() added to replace string concatenation within the for-loop, this is to improve client rendering speeds further. + Added better link to StringBuilder.

FURTHER UPDATE: Added suggestions by Hemlock:

  1. Removed use of globally scoped variable by defining var i = 0 in for-loop
  2. Pushed several strings at a time using multiple parameters of Array.push().
Steven de Salas
  • 20,944
  • 9
  • 74
  • 82
  • Much can still be done with this code. The worst problem is that `i` is never declared s/b `for var i=0;`. There is no need for the extra arrays, only one join is needed since push can accept multiple arguments. I don't understand why all that code is repeated to add the rows, it looks like that is just there to add alt styling. Can be done with one class on the tr and some decent css. Here is an example: http://jsbin.com/itego4/3/edit – Hemlock Jan 09 '11 at 13:52
  • @Hemlock, I made changes to push statement and locally scoped var but left if-then-else and css untouched, while I agree with you on that one I think its going beyond the scope of the question and changing css may have other side-effects. – Steven de Salas Jan 09 '11 at 14:40
  • very nice @steven - i had no idea i could attach long html strings like that ;) – Sagive Dec 04 '12 at 02:53
  • @StevendeSalas My tests on http://jsperf.com/string-concat-without-sringbuilder/3 show that a `StringBuilder` does more harm than good (method overhead), and that string concatenation is faster than the `Array.join` trick... care to comment on that? – Ruan Mendes Feb 27 '13 at 22:35
  • Hi Juan, while I cant vouch for the stats of your test I do know that since Google launched its Chrome browser with various JS engine compile-time performance optimizations in its like this one all the other browsers have followed suit. Perhaps this answer is only relevant to old versions of IE now. – Steven de Salas Mar 19 '13 at 22:58
3

one error i saw was that if (x = 0) should be if (x == 0), other than that it seems to work for me: http://jsfiddle.net/9vvJ6/

Alex
  • 1,327
  • 2
  • 11
  • 20
2

This line:

if (x = 0)

shouldn't it be

if (x == 0)

Might be the error here causing the rest of the script not being able to be executed.

mauris
  • 42,982
  • 15
  • 99
  • 131
  • if (x = 0) is correct, being syntax. It won't be cause an error. if assining 0 to x is successful, true... – miqbal Jan 09 '11 at 04:29
1

The problem is with .append jquery function. You can not use .html or .append function for long string. Instead you need to use .innerHTML

Try this

document.getElementById('example').innerHTML=theHTML.join('');
nsgulliver
  • 12,655
  • 23
  • 43
  • 64
  • This is not strictly true. As stated in the jquery documentation: "This [html] method uses the browser's innerHTML property" (http://api.jquery.com/html/#html2). – beterthanlife Jan 12 '17 at 13:48
  • Its worth noting that html() can be almost twice as slow as using innerHTML directly (http://stackoverflow.com/questions/1063303/jquery-html-acting-really-slow). – beterthanlife Jan 12 '17 at 13:59
1

If you're already using jQuery here, you might want to consider jQuery templates here, it would most likely clean up your code quite a bit and make your life easier. There can be performance tradeoffs (but I think string interpolation is faster than string concatenation in JavaScript, so this could even be faster?), but what you may or may not lose in performance, it's a much more elegant solution. You could keep your entire template in one place (maybe an external file, or parts of it hidden in the DOM of your document, I haven't used it enough to tell you what the best practice is), and then use jQuery templates to do the string replacement. There are multiple jQuery-based template frameworks, but the one I linked to is becoming an official part of jQuery.

carpeliam
  • 6,691
  • 2
  • 38
  • 42