10
\$\begingroup\$

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?

\$\endgroup\$
28
\$\begingroup\$

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]; 
    }
}
\$\endgroup\$
27
\$\begingroup\$

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

enter image description here

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.

\$\endgroup\$
  • \$\begingroup\$ Why do you think that undefined is a better default return value than null? \$\endgroup\$ – Guffa Oct 17 '14 at 7:53
  • 1
    \$\begingroup\$ @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. \$\endgroup\$ – dusky Oct 17 '14 at 9:28
  • \$\begingroup\$ @dusky: That depends on what the array contains and what the method is looking for, neither is clearly better than the other. \$\endgroup\$ – Guffa Oct 17 '14 at 12:31
  • \$\begingroup\$ @Gyffa undefined is more idiomatic, and shorter since it is the default assignment. var fe; <- fe is undefined. \$\endgroup\$ – konijn Oct 17 '14 at 13:10
20
\$\begingroup\$

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]
\$\endgroup\$
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – elclanrs Oct 16 '14 at 8:26
  • 3
    \$\begingroup\$ ECMAScript 6 is expected to introduce Array.prototype.find(), which is similar to your findFirst(). \$\endgroup\$ – 200_success Oct 16 '14 at 23:43
  • \$\begingroup\$ Thank you, it's not until now that I actually understand what the original code was doing... \$\endgroup\$ – Simon Forsberg Oct 18 '14 at 21:54
6
\$\begingroup\$

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

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.