Better way to write this code in functional manner using map and reduce?

Question

I have an array of items on which I have to perform 2 tasks:

1. Applying a function on the array item
2. Checking if the value is true or false.

The functional approach to solving this would be

var result = arr.map(function(item){return some_action(item);}).filter(function(item){return other_action(item) == true;});

But here the array arr is traversed twice in comparison to

var result = [];
arr.forEach(function(item){
 var x = other_action(some_action(item));
 if (x)
 result.push(x);
});

Isn't the functional approach bad in this case or am I not using map and filter the right way?

Answer 1

Your "functional" approach is overly complicated. Notice that this is completely equivalent:

var result = arr.map(some_action).filter(other_action);

I.e. If you're only delegating to another function, you can specify that function directly. Also, an == true test is superfluous.

Your "procedural" variant is not equivalent, that would have to be:

var result = [];
arr.forEach(function(item){
 var changedItem = some_action(item);
 if (other_action(changedItem))
 result.push(changedItem);
});

Note that both variants have the same algorithmic complexity, and that the cost of iteration is likely negligible compared with the cost of some_action and other_action.

Functional programming does not mean unreadable code. Even if you're not just delegating to another function, you could improve formatting, e.g. to

var result = arr.map(function (item) {
 return some_action(item);
}).filter(function (item) {
 return other_action(item);
});

• lovesh: sorry i mistyped the procedural version

Answer 2

Your loops are not equivalent. In the first one result will be arr mapped with some action, while in the second you could do that just using a filter.

The equivalent way of writing the forEach loop would be to use result.push(some_action(item)). Of course you would cache it as you use some_action(item) earlier in the function.

The equivalent way of writing your forEach loop with just filter would be

arr.filter(function(item){return other_action(some_action(item));});

Also, as this is code review I would like to point out you can probably write

arr.map(function(item){return some_action(item);})

As

arr.map(some_action) //note this is not equivalent if some_action can take multiple params.

Anyway, you are correct in that calling these loop methods will take O(n) time per call and it's a good idea to apply filters first to limit n. If you're looking for an interesting read take a look at the Lazy.js documentation.

Edit, just noticed, because you mention reduce, the way you'd write this with reduce is akin to you forEach way

arr.reduce(function(result, item){
 var item = other_action(some_action(item));
 if (item) result.push(item);
 return result;
}, []);

Answer 3

Don't do this:

 var result = arr.map(function(item){
 var result=some_action(item);
 if(other_action(result)){ 
 return result;
 }
 });

if other_action() return false, the item position in array will be undefined.

• megawac: When other_action(result) is falsy the mapped value will be undefined which is not desired. $.map will allow you to do that though