2
\$\begingroup\$

I've made a nice little function to return a NodeList array containing all the inputs that have a specified name attribute:

function GiveElementsByName(name) {
var finalOutput = [], times = document.getElementsByTagName('input').length, inOutput = 0, inAll = 0; //For use later.
while(times--)
{
if (document.getElementsByTagName('input')[inAll].name == name) //Apply to all inputs with specified name.
	{
	finalOutput[inOutput] = document.getElementsByTagName('input')[inAll]; //Array with all the matching inputs.
	inOutput++;
	}
inAll++;
}
return finalOutput;
}
function MyFunc() {
  GiveElementsByName("Lol1")[1].value = "I don't like potatos anymore";
}
<input type="text" name="Lol1" value="I like potatos">
<input type="text" name="Lol1" value="I like potatos">
<input type="text" name="Lol1" value="I like potatos">
<input type="text" name="Lol1" value="I like potatos">
<br><br><br><br>
<input type="text" name="Lol2" value="I like potatos more">
<input type="text" name="Lol2" value="I like potatos more">
<input type="text" name="Lol2" value="I like potatos more">
<input type="text" name="Lol2" value="I like potatos more">
<br><br><br><br>
<input type="button" value="Click me!" onClick="MyFunc()">

Could you help me shrink down the function and make it quicker?

\$\endgroup\$
1
\$\begingroup\$
  • Naming: Give… is unconventional; usually we say get…. The function only looks at <input> elements, so that should be part of the name. The "final" in finalOutput doesn't contribute to the clarity.
  • Performance: You're calling document.getElementsByTagName() many times. That DOM access is probably the most expensive call in your code, so you should do it just once and remember its result.
  • Initialization: This is one long line with poor readability:

    var finalOutput = [], times = document.getElementsByTagName('input').length, inOutput = 0, inAll = 0; //For use later.
    

    Eliminate as many variables as you can, and break up the rest into multiple lines of code.

  • Looping: A common idiom for a loop that runs n times is

    for (var i = 0; i < n; i++) {
        …
    }
    

    There are times when while (times--) { … } is appropriate, but this is not one of them, since you need an inAll that counts up anyway.

  • Building the array: If you use Array.push(), then you don't have to keep track of inOutput.
function getInputElementsByName(name) {
    var inputs = document.getElementsByTagName('input');
    var out = [];
    for (var i = 0; i < inputs.length; i++) {
        if (inputs[i].name == name) {
            out.push(inputs[i]);
        }
    }
    return out;
}
\$\endgroup\$
2
\$\begingroup\$

The GiveElementsByName function is poorly indented. And the variable declarations should not be only a single extremely long var line, but each variable should be on its own line. It's better like this:

function GiveElementsByName(name) {
    var finalOutput = [],
        times = document.getElementsByTagName('input').length,
        inOutput = 0,
        inAll = 0;
    while (times--) {
        if (document.getElementsByTagName('input')[inAll].name == name) {
            finalOutput[inOutput] = document.getElementsByTagName('input')[inAll]; 
            inOutput++;
        }
        inAll++;
    }
    return finalOutput;
}

It seems to me that there is an easier way to do this using document.querySelectorAll, which should be supported by modern browsers:

function GiveElementsByName(name) {
    return document.querySelectorAll('input[name=' + name + ']');
}
\$\endgroup\$
1
\$\begingroup\$

The big thing would be to call getElementsByTagName no more than you need to. In your method, you call it once to get the length, then twice more for every result (once to get the name, then again to add it to your array). This could require the code to parse the DOM an excessive number of times.

In the example code below, I call document.getElementsByTagName only once.

function GiveElementsByName(name) {
    var finalOutput = [], tags = document.getElementsByTagName('input'), inOutput = 0, inAll = 0, times = tags.length; //For use later.
    while(times--){
        if (tags[inAll].name == name){ //Apply to all inputs with specified name.
            finalOutput[inOutput] = tags[inAll]; //Array with all the matching inputs.
            inOutput++;
        }
        inAll++;
    }
    return finalOutput;
}

However, since you're really looking for two different properties of the elements (the tag name "input" as well as the name attribute) you could try using document.querySelectorAll instead of getElementsByTagName, which allows you to specify a complex CSS selector.

At a glance, your function could thus be replaced with:

function GiveElementsByName(name) {
    return document.querySelectorAll("input[name="+name+"]");
}
\$\endgroup\$
  • \$\begingroup\$ Ermmm... Hmm... Realizes he's stupid... Well, I choose the fancy way! \$\endgroup\$ – The SuperCuber Feb 20 '15 at 18:35
  • \$\begingroup\$ Not stupid, just taking a different approach. In fact, depending on the browser and the element(s) being selected, getElementsByTagName can be faster than querySelectorAll. Check out this jsperf test for some comparisons. \$\endgroup\$ – Thriggle Feb 20 '15 at 18:52

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.