Improving on for loops

[Paul_Wilkins]

If you ever lint your code, you might find that for loops aren’t liked by JSLint and other linters.

For example, the Kingdom Builder randomizer has a section that clears all of the fields, that includes the following loop:

    for (var i = 0; i < 4; ++i) {
        var t = document.getElementById("t" + i);
        t.value = '';
        t.style.backgroundColor = '';
    }

Cleaning up other issues with the code

Other issues exist such as the single quotes needing to be double quotes, the vars needing to be out of the for loop, and the ++i needing to be i += 1 instead, and after fixing that we have:

    var i;
    var t;
    for (i = 0; i < 4; i += 1) {
        t = document.getElementById("t" + i);
        t.value = "";
        t.style.backgroundColor = "";
    }

Unexpected for

So why does the linter say “Unexpected ‘for’.” ?
It’s because virtually all of the time, there are better ways to do it.

Frequently loops occur over an array of items, which has its own separate forEach method on the array.
In this case we aren’t using an array, so we can replace the for loop with a while loop instead, and make improvements from there.

    var i = 0;
    var t;
    while (i < 4) {
        t = document.getElementById("t" + i);
        t.value = "";
        t.style.backgroundColor = "";
        i += 1;
    }

That’s not the end of things though. The while loop is just a stepping stone to better and more structured code.

Remove magic values

Why are we checking that i is less than 4? That’s a rather arbitrary value, also called a magic number as it’s a unique value with an unexplained meaning. After investigating the HTML code we find that there are four input fields that are being looped through:

	<p>
	  TASKS: <br>
	  <input type="text" id="t0" readonly="readonly"> 
	  <input type="text" id="t1" readonly="readonly"> <br>
	  <input type="text" id="t2" readonly="readonly">
	  <input type="text" id="t3" readonly="readonly"> <br>
	</p>

So the magic number 4 is to let us loop over each of these input fields.

Solution 1: Check that input exists

One possible solution is to instead check if the element exists before carrying on:

        var i = 0;
        var t = document.getElementById("t0");
        while (t) {
            t.value = "";
            t.style.backgroundColor = "";
            i += 1;
            t = document.getElementById("t" + i);
        }

That results in the while loop ending as soon as an element can’t be found, and can more flexibly handle changes to the HTML code.

Solution 2: Directly get an array-like nodeList of the inputs

Another potentially better solution is to get the elements straight from the HTML code using the starts with (^=) selector, and loop over those instead:

        var tasks = document.querySelectorAll("input[id^=t]");
        tasks.forEach(function (task) {
            task.value = "";
            task.style.backgroundColor = "";
        });

But, if other elements start with t we have more trouble.

Solution 3: (preferred) Directly target the inputs

A better solution is to place an HTML classname on the tasks, and loop over the input fields there instead:

	<p class="tasks">
    	...
	</p>

We can now easily select all of the inputs in the tasks area, and loop through them:

        var tasks = document.querySelectorAll(".tasks input");
        tasks.forEach(function (task) {
            task.value = "";
            task.style.backgroundColor = "";
        });

Structured code replaces for loops

The code that we ended up with is much more easily capable of handling future changes to the code. Linters complains about for loops, because they tend to block any further improvements to the code.

Without the for loop, we are much more easily able to structure the code to achieve more reliable and robust outcomes.