# 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?

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? Commented Oct 17, 2014 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. Commented Oct 17, 2014 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. Commented Oct 17, 2014 at 12:31
• @Gyffa undefined is more idiomatic, and shorter since it is the default assignment. var fe; <- fe is undefined. Commented Oct 17, 2014 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. Commented Oct 16, 2014 at 8:26
• ECMAScript 6 is expected to introduce Array.prototype.find(), which is similar to your findFirst(). Commented Oct 16, 2014 at 23:43
• Thank you, it's not until now that I actually understand what the original code was doing... Commented Oct 18, 2014 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