# Possible antipattern: looping until a search condition is met

Whenever I write a loop, I always use closure if it's appropriate by way of a self-executing anonymous function. I will typically write my searching loops as follows:

var i = 0, length = myArr.length,
fe = null;

for (; i < length; i++) {
if ((function (el) {
if (el.searchCondition) {
fe = el;
return true;
}
}(myArr[i])) === true) {
break;
}
}

return fe;


I thought it was minimal code and pretty clean. I know it's not the easiest to read - but is there any reason why I should not be doing it this way?

• There is practically no reason to use loops at all, when we have abstractions like map, filter, reduce, etc... – elclanrs Oct 16 '14 at 8:08
• I didn't think the latter two were ECMA standards? Could you give an example of each perhaps to show how it can be done? – keldar Oct 16 '14 at 8:10
• What is this loop for? Can you give more context; an example of input and output? – elclanrs Oct 16 '14 at 8:11
• @keldar They're in ECMAScript 5.1 – dusky Oct 16 '14 at 8:11

I know it's not the easiest to read - but is there any reason why I should not be doing it this way?

You answered your own question already. It's not easy to read, which is a very good reason not to do it this way.

In addition to the readability, your code will (probably, I didn't profile it) also perform worse than the simple solution (because of the extra function call).

So you have two points against this pattern. What are the advantages?

Two other points: if (exp === true) adds extra complexity. I would just write if (exp), and let the function return false. And you should assign i right were you need it, inside the loop.

If you consider all those points, your code might look like this:

var length = myArr.length,
fe = null;

for (var i = 0; i < length; i++) {
if (myArr[i].searchCondition) {
fe = myArr[i];
break;
}
}

return fe;


If you don't like that the array is accessed twice, save the value in a temporary variable. Or just return directly like @dusky did:

var length = myArr.length;

for (var i = 0; i < length; i++) {
if (myArr[i].searchCondition) {
return myArr[i];
}
}


Possible anti-pattern is quite an understatement. There is so much wrong with this that I had to get creative to get over it.

The suggestion by @dusky are excellent on how to fix it, but I did not feel he was clear enough on how terrible that code is.

• Why do you think that undefined is a better default return value than null? – Guffa Oct 17 '14 at 7:53
• @Guffa null => the value was set intentionally (to null), undefined => it wasn't set at all. Since an element was not found I would expect undefined. – dusky Oct 17 '14 at 9:28
• @dusky: That depends on what the array contains and what the method is looking for, neither is clearly better than the other. – Guffa Oct 17 '14 at 12:31
• @Gyffa undefined is more idiomatic, and shorter since it is the default assignment. var fe; <- fe is undefined. – konijn Oct 17 '14 at 13:10

Readability is very important. You're code does a simple thing but it takes time to actually understand what it's doing.

function findFirst(array, filterFunc) {
var length = array.length;
for (var i=0; i < length; i++) {
if (filterFunc(array[i])) {
return array[i];
}
}
}


Or simply

myArr.filter(filterFunc)[0]

• It does take time to understand, I gave up after a couple reads, just posted a comment for further explanation. Other than the one-space indent this looks much better. – elclanrs Oct 16 '14 at 8:26
• ECMAScript 6 is expected to introduce Array.prototype.find(), which is similar to your findFirst(). – 200_success Oct 16 '14 at 23:43
• Thank you, it's not until now that I actually understand what the original code was doing... – Simon Forsberg Oct 18 '14 at 21:54

One reason would be performance. Normally performance shouldn't be much of an issue until it's actually a problem, but loops are where performance degrades quickly.

The performance impact will of course differ depending on what you do in the code, but this test shows that the code without the function wrapper runs more than 10 times faster:

http://jsperf.com/loopclosure