Big lessons from a small bug

TLDR; use native methods to reduce complexity, good test data requires design.

What's wrong here?

I recently came across this question on StackOverflow. A developer was struggling with this bit of code.

The function is supposed to receive an array of integers, and return an array of only the odd integers. There is something wrong with it, do you see it?

``````const testA = [1, 2, 3, 4, 5, 6, 7, 8, 9];

function filterOddValues(inputArray) {
let outputArray = [];
for (let i = 0; i < inputArray.length; i++) {
if (i % 2 !== 0) {
outputArray.push(inputArray[i]);
}
}
return outputArray;
}

console.log(filterOddValues(testA));``````

As written, this function will return: [2, 4, 6, 8].

Oops! We were supposed to get an array of the odd values but we got back all the even values.  That's not what we wanted.

Is it the math?

Modulating an odd value by 2 should return a 1.

``````const testForOdd = function (a) {
return (a % 2);
};
console.log(testForOdd(3)); // 1``````

In our example, yep we are modulating our value by 2. That's good.

Is it our condition?

We are testing the result of our modulation, it looks correct, it shouldn't be zero, but for play, because this is what beginners do, let's just reverse it.

``````const testA = [1, 2, 3, 4, 5, 6, 7, 8, 9];

function filterOddValues(inputArray) {
let outputArray = [];
for (let i = 0; i < inputArray.length; i++) {
if (i % 2 === 0) {  // reversed the condition
outputArray.push(inputArray[i]);
}
}
return outputArray;
}

console.log(filterOddValues(testA));``````

The results are: [1, 3, 5, 7, 9].

Oh crap! That's the right answer! But it can't be, reversing that condition shouldn't work. And a beginner asks themselves, do I even understand math and JavaScript?? Or worse, they say, ah, it works!!! I've got it, and then they push their code into the app.

For the want of a nail

You've probably already seen it. But if not, here's the reveal:

``````const testA = [1, 2, 3, 4, 5, 6, 7, 8, 9];

function filterOddValues(inputArray) {
let outputArray = [];
for (let i = 0; i < inputArray.length; i++) {

// should be modulating inputArray[i], not i
if (i % 2 !== 0) {
console.log(i);   // 1, 3, 5, 7
console.log(inputArray[i]); // 2, 4, 6, 8
outputArray.push(inputArray[i]);
}
}
return outputArray;
}

console.log(filterOddValues(testA));``````

The bug is that the function modulated the wrong value. It tested i, which is the index of the array, rather than inputArray[i] which is the actual array element's value.

This is not a truly hard bug to find, most experienced coder's eyes will find it quickly. However, what's particularly nasty about the example is that the test data was completely misleading the developer. If you look at the code snippet directly above i  and inputArray[i] are just off by one. This means that because the dev is testing i that the opposite of the mathematically correct condition (=== vs !==) gave him the right answer, in this case.

Lessons Learned

Testing your code is the right thing to do. You should begin writing a test just before you start writing your class, object, or function. However, if your test data is not designed as well as the test itself, you can have passing tests for very buggy code.

Here, the dev should have used a random set of non-sequential integers. This would have meant that at least he didn't get a false positive. In this example this is just small stuff, but the importance only grows with the scale of your app and the complexity of your data.

On a recent enterprise level project, a major customer-facing web application was being built and all the staging, test, and Q&A work was being done with the same test customer account. Bad idea! Not every customer is configured the same, and thus does not have the same user experience. So you need many test customer accounts to test those various scenarios.

So think carefully about the data you use as the basis for your tests. Do not use the simplest case, or the same case over and over again. You must think about the variety, complexity, and volume of your data.

Use Native Methods Where Available

There is a significant takeaway here for coding style as well. The entire example above could have been coded simply as:

``````const filterOddValues = (test) => {
return test.filter(a => a % 2 !== 0);
}``````

This relies on the native JavaScript filter method and it works. It's beautifully shorter, it has none of those nasty, poorly named vars such as ijk, or xyz, etc. that tend to get used in for... loop iterators which cause so many bugs. So, avoid them if at all possible and use native Object and Array methods.

Both of these lessons may seem small, but piled up, over and over, it is things like this which make your code usable, scalable, and testable, or the opposite of those qualities.

Here's the Codepen with the buggy example and a better way: