1
\$\begingroup\$

The program we are working at has the following function:

function loadItem(arrayValues,arrayNames) {
    var j=document.getElementById('itemSelect').options.length;
    for(var i=0;i<arrayValues.length;i++) {
        if(!existsElement(document.getElementById('itemSelect'),arrayValues[i])) {
            document.getElementById('itemSelect').options[j]=new Option(arrayValues[i]+" - "+arrayNames[i],arrayValues[i],null);
            j++;
        }
    }
}

This fills a list that is later used to filter a search.

It was originally designed for small arrays (up to 25 elements) and it worked perfectly.

Now, I'm trying to use it to fill with the whole contents of a SQL table (about 9500 elements), I take the data from the DB with Java, transfer it to Javascript via AJAX and parse that List into two arrays that can be used with this function. That takes about a second, but then this function takes about 20 seconds to complete.

Why is it that slow, and what can I do to improve it?

\$\endgroup\$

closed as off-topic by t3chb0t, Toby Speight, IEatBagels, Stephen Rauch, Dannnno Oct 2 '18 at 16:24

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – t3chb0t, Toby Speight, IEatBagels, Stephen Rauch, Dannnno
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 2
    \$\begingroup\$ Please reveal more details about how this method is used and what else your page is doing etc. Maybe there are some automatic event handlers... who knows. We need more info. \$\endgroup\$ – t3chb0t Oct 2 '18 at 10:45
  • \$\begingroup\$ I think I see where this is going... \$\endgroup\$ – FreezePhoenix Oct 2 '18 at 11:21
  • \$\begingroup\$ Hey, the code for existsElement would be useful. \$\endgroup\$ – FreezePhoenix Oct 2 '18 at 12:00
  • 1
    \$\begingroup\$ As it currently stands, this question's title leaves something to be wanted. The title should explain what the code does, not what is wrong with it. \$\endgroup\$ – FreezePhoenix Oct 2 '18 at 12:44
2
\$\begingroup\$

Main issues

Access

  • DOM operations take time. You're selecting the element N^2 + 1 times. This can be reduced to 1: document.getElementById('itemSelect')
  • You're constantly getting arrayValues[i], which only needs to be done once.

Style

  • You should put spaces after commas: arg, arg2
  • You should put spaces before and after operations, for readability: var name = value
  • Spaces after semicolons: i < length; i++

Rewrite

function loadItem(arrayValues, arrayNames) {
    var itemSelect = document.getElementById('itemSelect'),
        j = itemSelect.options.length;
    for(var i = 0, length = arrayValues.length; i < length; i++) {
        let val = arrayValues[i];
        if(!existsElement(itemSelect, val)) {
            itemSelect.options[j] = new Option(val + " - " + arrayNames[i], val, null);
            j++;
        }
    }
}
\$\endgroup\$
0
\$\begingroup\$

DOM operations are costly, so use variables.

var is=document.getElementById('itemSelect');
for(var i=0;i<arrayValues.length;i++) {
    if(!existsElement(is,arrayValues[i])) {
        is.options.push(new Option(arrayValues[i]+" - "+arrayNames[i],arrayValues[i],null));
    }
}
\$\endgroup\$
  • \$\begingroup\$ This answer assumes that the .options property is empty, which may not be what the author wants. \$\endgroup\$ – FreezePhoenix Oct 2 '18 at 11:34
  • \$\begingroup\$ @FreezePhoenix I am no JS expert. Is it because I also changed [length] to .push? \$\endgroup\$ – Joop Eggen Oct 2 '18 at 11:49
  • \$\begingroup\$ [index] sets the value for that index, .push appends a value to the end of it. Yes, it is because of that change. \$\endgroup\$ – FreezePhoenix Oct 2 '18 at 11:59
  • \$\begingroup\$ @FreezePhoenix then this should be the same functionality as that of the OP's: if not existing, push a new one to the options. BTW your code is more nicely styled. \$\endgroup\$ – Joop Eggen Oct 2 '18 at 14:08