Is it bad practice to use JavaScript Array.prototype.map with scoped variables

Question

I know that map() is meant to iterate lists, run a function on each member and return (or not return) a value as a list item. But what if I use it the same way as I would use forEach()?

Example:

var stuff = [1, 2, 3];

var newStuff = {};
var moreStuff = [];

stuff.forEach(function(value, key){
    newStuff[value] = {'value' : value};
    moreStuff.push({'other_stuff': value});
});

$('body').append('<div>1. ' + JSON.stringify(newStuff) + '/' + JSON.stringify(moreStuff) + '</div>');

//vs.

newStuff = {};
moreStuff = [];

stuff.map(function(value, key){
    newStuff[value] = {'value' : value};
    moreStuff.push({'other_stuff': value});
});

$('body').append('<div>2. ' + JSON.stringify(newStuff) + '/' + JSON.stringify(moreStuff) + '</div>');

results...

1. {"1":{"value":1},"2":{"value":2},"3":{"value":3}}/[{"other_stuff":1},{"other_stuff":2},{"other_stuff":3}]
2. {"1":{"value":1},"2":{"value":2},"3":{"value":3}}/[{"other_stuff":1},{"other_stuff":2},{"other_stuff":3}]

https://jsfiddle.net/0n7ynvmo/

I'm pretty sure, async considerations are the same (and might botch this example), and results are the same, but for the sake of discussion of best practices, what are the drawbacks to one way vs. another? Is it an abuse of the map() method to change a previously scoped variable within the function and not actually return anything? Is the correct answer simply a matter of discretion?

There are obvious drawbacks, like allocating a new array and making the code confusing but your question is puzzling: why would you even want to use map instead of forEach when you're not interested in the result ? And why aren't you just building moreStuff as a returned value of map in this precise case ? — Denys Séguret
– Denys Séguret, Commented Aug 5, 2016 at 15:20
ya, a bit confusing. I guess technically, I am interested in the result. answer below makes it clearer, as the intention is there, but the implementation could be improved. — 4m1r
– 4m1r, Commented Aug 5, 2016 at 15:23
I guess it would be more clear without two changing object types in the implementation. — 4m1r
– 4m1r, Commented Aug 5, 2016 at 15:25
I think you had misunderstood how map worked and that tymeJV's answer probably addressed this confusion, but just to make sure : map will always return a transformed array, whose values will be the result of the application of the provided function to the original array's values. If said function does not explicitly return any value, it will be an array of undefined values (as implicitly returned by the function) — Aaron
– Aaron, Commented Aug 5, 2016 at 15:25
.map() is your friend if you want to minimize side-effects. Using it this way - you are going out of your way to remove this benefit from .map() and make your code harder to understand. — Yasin Yaqoobi
– Yasin Yaqoobi, Commented Aug 5, 2016 at 15:26

S McCrohan · Accepted Answer · 2016-08-05 15:23:46Z

4

It works. However, applying Principle of Least Astonishment here suggests that it's better to use the function whose known / primary use matches your intent rather than using a different one 'off-label'. There's no TECHNICAL reason to use .forEach() over .map() here that I know of, but because generating side-effects out of .map() is an uncommon pattern, it's going to make your code less legible than it would be if you used .forEach().

If you're working in a team environment where many hands will touch your code, then it's worth being conventional in order to maximize future understanding of intent.

answered Aug 5, 2016 at 15:23

S McCrohan

6,7011 gold badge33 silver badges39 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

4m1r Over a year ago

agreed, thanks. I really like @tymeJV's answer because it does that, improving both the legibility and the intended implementation.

Aaron Hammond · Accepted Answer · 2016-08-05 15:26:40Z

4

It's really, really poor practice to pass a callback with side effects to map, filter, reduce, etc. Most folks reading your code would expect the callback to return the new array element and do nothing else.

Something like the following is more tightly tailored to your needs and more readable.

var moreStuff = stuff.map(function(value) {
    return { otherStuff: value };
});

var newStuff = {};
stuff.forEach(function(value) {
   newStuff[value] = { value: value };
});

answered Aug 5, 2016 at 15:26

Aaron Hammond

6175 silver badges9 bronze badges

1 Comment

4m1r Over a year ago

agreed and good solution, but I think @tymeJV's solution does this in a more performant way and demonstrates that there's not really anything wrong with side effects from a map... or is there?

tymeJV · Accepted Answer · 2016-08-05 15:18:11Z

-3

Not really many drawbacks - except in forEach you're not returning a new list. But in your case, you want a new list, so use .map

var newStuff = {};

moreStuff = stuff.map(function(value, key){
    newStuff[value] = {'value' : value};
    return {'other_stuff': value};
});

answered Aug 5, 2016 at 15:18

tymeJV

105k14 gold badges165 silver badges158 bronze badges

Collectives™ on Stack Overflow

Is it bad practice to use JavaScript Array.prototype.map with scoped variables

3 Answers 3

1 Comment

1 Comment

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

1 Comment

1 Comment

Comments

Your Answer

Sign up or log in

Post as a guest

Related