patternjavascriptMajor
Possible antipattern: looping until a search condition is met
Viewed 0 times
loopingantipatternmetconditionsearchuntilpossible
Problem
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:
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?
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?
Solution
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 you consider all those points, your code might look like this:
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:
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];
}
}Code Snippets
var length = myArr.length,
fe = null;
for (var i = 0; i < length; i++) {
if (myArr[i].searchCondition) {
fe = myArr[i];
break;
}
}
return fe;var length = myArr.length;
for (var i = 0; i < length; i++) {
if (myArr[i].searchCondition) {
return myArr[i];
}
}Context
StackExchange Code Review Q#66839, answer score: 28
Revisions (0)
No revisions yet.