Possible antipattern: looping until a search condition is met

Question

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?

Answer 1

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]; 
    }
}

Answer 2

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.

Answer 3

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

Isn't that easier to read?

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]

Answer 4

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