Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share|improve this question

3 Answers 3

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);
});
share|improve this answer
    
sorry i mistyped the procedural version –  lovesh Jan 31 at 12:52

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;
}, []);
share|improve this answer

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.

share|improve this answer
1  
When other_action(result) is falsy the mapped value will be undefined which is not desired. $.map will allow you to do that though –  megawac Feb 3 at 4:17
    
you are right. I am not aware of it. thanks. –  caoglish Feb 3 at 7:12

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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