DEV Community

Stop abusing .map()!

Pan Seba on September 26, 2021

Every once in a while when I do code review or visit StackOverflow I stumble upon code snippets that look like this: const fruitIds = ['apple', ...

Read full post

Phantz • Sep 27 '21

It should be noted that forEach is no longer the only alternative. Since ES7 or so, with the addition of methods like .entries on pretty much all collections, a for..of can do everything a forEach can.

Nailed the point in the head about map though. Mapping is not supposed to have side effects. A programmer should be able to know the gist of a higher order list operation by looking at the method call. When I see .map, I automatically assume it's a mapping. I don't want to find out later that it's actually modifying some external state. Seeing this kind of code smell in the wild is far too common, unfortunately.

Petros Kyriakou • Sep 27 '21

.entries should be used sparingly as its performance is bad for large collections. The fastest being for loop.

Phantz • Sep 28 '21 • Edited

Actually, I was referring to the newer .entries methods. You know, Array.prototype.entries, Set.prototype.entries, Map.prototype.entries etc. They all return iterators and will not be slow.

Meanwhile the old and outdated Object.entries goes over the entire collection to build and return the array, which will then be iterated over by the user again. A truly unfortunate state. But hey, at least they are on the right path on the modern .entries impls!

Since we're on the topic, here's an efficient Object.entries impl instead-

function* objEntries<T>(x: Record<string, T>): IterableIterator<[string, T]> {
  for (const k in x) {
    yield [k, x[k]];
  }
}

playground

Petros Kyriakou • Sep 30 '21

yes i thought you were talking about Object.entries

Andy Chen • Sep 30 '21

It is quite true that functional approaches in JS are generally slower than the imperative approach, but I do think that it improves readability of the code and the performance overhead can usually be ignored......

Ashley Sheridan • Oct 20 '21

Performance overhead can absolutely be ignored... if you don't care about the majority of your users. Usage is moving more and more to mobile devices, which are less powerful than desktops and laptops. They are also battery powered, and the more work the mobile browsers are being asked to do, the more it eats into that battery life.

Is readability of the code (and map is really no more readable than a foreach) really more important than the user base of the final product?

Max Waterman • Sep 30 '21

IMO, you should move as much code into the js engine, and using js looping constructs does not do that. Also, for loops are...not as simple as you might have thought...they have been completely screwed up by the people who decide what JS/ES is:

youtube.com/watch?v=Nzokr6Boeaw

Before, they used to be one of the simplest and most elementary ways to loop.
In JavaScript, now, I never use them. Frankly, these days, there's always something better.

Why couldn't they just do this:

// for( a; b, c ) { d }
{
  a;
  while (b) {
    d;
    c;
  }
}

Assuming I got that correct - you get the idea anyway.

Amar Bugarin • Oct 1 '21

Can you elaborate on the performance part?

Tu Le Thanh • Sep 28 '21 • Edited

Could you please tell what is the properly way to have side effects with each element in an array? I currently using side effects with map and promise.all

Phantz • Sep 28 '21

Could you give an example of the usage that you need clarification with? Generally, if you're iterating over an array purely to perform side effects and are not interested in building a new array after mapping a function on each element - you would use a regular for..of loop.

tanth1993 • Sep 28 '21

developer.mozilla.org/en-US/docs/W...
in this site, you will see, map function calls the callback() as synchronous function.
Therefore, there is no side effect items will be resolved.
You can use for or for... of to solve side-effect arrays

JoelBonetR 🥇 • Sep 27 '21 • Edited

Array.prototype.map is so used because in React you need -usually- to print a key (which is usually the id property value) and the value when rendering an array of objects (that comes from a fetch most of the time), like that:

return ( 
  <> 
    <div className="content"> {data.map( item => {
      return <p key={item.id.toString()}> {item.value} </p>;
    })}
  </>);

of course it can be achieved in several ways but this is a one-liner straightforward sort of standard for this kind of iterations. This plus the fact of React being the #1 tool (library in this case) to deal with reactive UI... we got a starter point on this trend.

In terms of performance, consuming memory with this kind of array methods is not the best you can do as you said but at the end the difference between this and other "go-to"s is usually negligible.
If you work with pagination as you should it will iterate over... 10 elements? 50? that's really nothing in computation terms.

I mean.. if you work on frontend simply filter your call with ?page=1 and make the backend return 10 elements or 12 or 25 or whatever number is good for your use case. If you are working on backend, you should ask your database with a start point and a limit according to this filter (?page=2 at a reason of 10 per page will be results from 10 to 20) instead making stupid things such select all from database and slicing results in the code.

That being said, there are few places where you should deal with an array of 10.000-100.000 items or more in the code, and if you are in a place that needs that you'll probably be in backend dealing with aggegated data and using the thing that performs best for that amount of data (which usually is not JS and definitely is not cloning arrays using map or equivalent methods on other languages).

There are many things with a higher impact on performance such creating wrappers to fetch content in a single call for all page's components in load time and I see few people doing that so please, first things first.

-In my opinion- you can keep using map unless you work on a max performance driven project (in which case you'll probably use custom data structures but well).

There are many things that should be in the first page of best practices when developing a website: enable HTTP/2 in your server, wrap calls whenever possible, avoid unnecessary/non-critical re-renders, learn how to deal with the cache and so on.

And if we talk about performance and memory usage then we'll need to put over the table some other methods such while, do...while, for...of, for...in, for and define whether is better to use each and I'm not the one making this huge job tbh 😆

Les Orchard • Sep 27 '21 • Edited

Array.prototype.map is so used because in React you need -usually- to print a key (which is usually the id property value) and the value when rendering an array of objects (that comes from a fetch most of the time), like that

The way Array.map() is used in your example in React is appropriate usage: It's transforming a list of data objects into a list of elements. It's neither throwing away the result of the map nor altering the data objects on the way through.

The main point of this article is that the "abuse" of map is consuming memory for data that never gets used, because you just wanted a simple loop (e.g. to set an attribute on a collection of elements, not derive a new collection)

Otherwise, yes, if you're trying to render 10k-100k items in one go, you're in need of another solution entirely. But that's not really the point of this article.

JoelBonetR 🥇 • Sep 27 '21 • Edited

Didn't say the opposite, isn't it?
I'd just meant to add a bit of context and insights to the articles topic. As you can see in the comments below, there's people that reads it all and say "ok so it's just bad to use" or "ok, so people who uses .map() are just bad devs" and so on. Some of them comment and others just leave with this idea, that's the reason I try to add some context and salt into this sort of "STOP USING xxxxx" topics.
*Note on here: The OP did a good job saying STOP ABUSING and not STOP USING, but even that, people still falls on this counterproductive thinking.

Owen Melbourne • Sep 29 '21

I understood the article was saying "Stop using .map() incorrectly!"

.map() is 100000000% valid, when used correctly.

Maybe a language barrier thing.

Martin Omander • Sep 27 '21

I agree with the main sentiment of the article: map() should be used for mapping and not for iterating. Why? In my opinion, to make the intention of your code clearer to your co-workers.

Then there is the article section about performance. Whenever an article mentions performance, I wish that the author would back it up with test results. Creating and throwing away an array of undefineds is not optimal, but is it noticeable? Will an array of 5,000 elements cause trouble? How about an array of 5 million elements? If it does cause trouble, what kind of trouble? The author's point about performance would be stronger if it were backed up by real test results.

Hope to see more articles from you in the future, Sebastian!

lionel-rowe • Sep 27 '21

In 99% of cases, the performance difference will be too small to matter. The more important argument for forEach (or for... of) in those 99% of cases is that it's explicit about what it's doing.

xtofl • Sep 27 '21

I can imagine a Javascript optimizer looking at { xs.map(f) } and deciding that, since the mapped return value is not used, and there can be no side-effect from f, it might as well omit executing the whole block.

Or is there something in the standard that enforces map to actually run?

Phantz • Sep 27 '21

It would be valid optimization in certain languages. But V8 won't do this unless it can prove that the mapping function is pure. That's very difficult unless certain functions are explicitly marked as "pure". As far as I'm aware of, V8 employs no such technique. So V8 would rather lean on the safe side and not erase operations.

But in general, optimizations must not modify perceivable behavior of a program. I've noticed the exact definition of "perceivable" be different in different languages and implementations though. In the context of JS and V8, I've never seen redundant mappings get optimized away.

Dmitry Boichev • Sep 27 '21

Probably but it’s negligible

antonioaltamura • Oct 3 '21 • Edited

the article would have been complete with a bechmark.
Here we are jsbench.me/ujkub8rcv2/1
forEach seems to be the fastest in that snipped. map seems to be around 20% slower and for..of 15% slower.

Darryl Hodgins • Sep 29 '21

map with side effects… yes, that's bad practice. But one could use map here. It might not be necessary in the trivial case, but if we want to throw a filter in there as a sanity check, this makes for readable code:

const fruitIds = ['apple', 'orange', 'banana', 'carrot'];  // carrot is not a fruit

fruitIds.map(
  (id) => document.getElementById(`fruit-${id}`)
).filter(
  (element) => !!element
).forEach(
  (element) => element.classList.add('active')
);

Max Waterman • Sep 30 '21

Why not just the following?

const fruitIds = ['apple', 'orange', 'banana', 'tomato'];  // tomato is a fruit

fruitIds.forEach((id) => {
  const element = document.getElementById(`fruit-${id}`);
  element?.classList.add('active');
});

Is that not equivalent, clear, and only iterates the list once instead of ~3 times?
What are the pros and cons of chaining array methods like the above? Perhaps easier to add another method, eg sort(), or some other filter()...but in this instance, not so much, imo.

Petros Kyriakou • Sep 27 '21

I think the main point here is that .map() is used to do a transformation on the data and return that as a new array.

it should not be considered a loop mechanism.

fawazsullia • Sep 29 '21

I'm guilty of doing this. Makes total sense, since the arrays I'm iterating are mostly objects and are huge. Thanks for the article.

Brian McBride • Sep 29 '21

I guess incorrect use of Map is really only an issue if it is harming the app performance. Someone dealing with huge datasets will see that. Otherwise the returned array is going to be garbage collected after the containing function exits.

Still, better to use the right tools. And you know, there is a lot of docs for that!
developer.mozilla.org/en-US/docs/W...

It is good to know many of these. I mean, who uses copyWithin()? I mean, handy, but I haven't needed that edge case myself. includes() and some() are ones I don't see used often, but are handy (some is sort of like findIndex but returns boolean).

Max Waterman • Sep 30 '21

I guess incorrect use of Map is really only an issue if it is harming the app performance.

I think it's more of an issue on developer performance - ie the word 'map' has a meaning that the developer understands, and if they read 'map', the notice that the results of the map are thrown away, it leads to a brain fart (aka 'wtf?'). forEach has no such smell.

Larson • Sep 27 '21 • Edited

forEach is Running checks for each value like any array function. But the loop inside is an while loop and the fastest atm.

hyungjunk • Sep 28 '21 • Edited

I totally agree to the point of the post. One thing I wonder is if map is used just like the bad practice example, when does garbage collector collects that isolated, unused array? will it exists for long?

Lewis Cowles • Sep 27 '21

😑 honestly, while you're not wrong, do you not have anything more important to complain about?

I see much worse than this on a daily basis.

Jessy • Oct 26 '21

We have to start somewhere. Knowing the difference is important and how we become better developers. Not caring is how we keep staying messy and careless in our craft. Keep learning and thinking about everything. This may not be solving problems for the client/product/customer of the application its written in, but it might be solving or clearifying the user experience of the developers that have to work with the code; an aspect that seems to be left behind pretty often by developers.

Marcos Borunda • Sep 28 '21

😑 honestly, while you're not wrong, do you not have anything more important to complain about?

I see much worse than this on a daily basis.

Younes • Sep 28 '21

There is also a performance cost to consider when looping:

leanylabs.com/blog/js-forEach-map-...

Pranay Singh • Sep 29 '21

Nice Post

tanth1993 • Sep 28 '21

I totally agree. when I need return something I will use map(), otherwise I use forEach()

Christopher Campanale • Sep 28 '21

Stop telling me what to do! I do what I want!

😘

Yann MICHAUX • Sep 27 '21

If someone use .map() to iterate over array, they're just bad developers...

JoelBonetR 🥇 • Sep 27 '21 • Edited

If it were so bad why they implemented it in the language API in the first hand?

There are many details in tech and you need to know why they exists. A method that copies an array with the desired data is not necessarily bad unless you use it as an average iterator instead, its what we call a reducer and it could be fine for tones of use cases.
Cloning an array is not bad itself, the bad thing is to not delete them when you don't need them anymore.

Let's add an example:

If you get a response from a REST call, for example you can get something like:

users = [{
  id: 1,
  name: 'John',
  surname: 'Doe',
  birthdate: '10-11-1980',
  email: 'john.doe@gmail.com',
  ...
}, {
  ...
}]

If you only need to propagate the ID, the first Name character and the surname to build something like:

<span id="user.id"> user.name.charAt(0).toLowerCase() + user.surname.charAt(0).toUpperCase() + user.surname.slice(1).toLowerCase() </span>

so you get

<span id="1"> J. Doe </span>

Why you want to propagate the entire object in the first hand?
simply create a new array with the desired props using map and use it! you can free the old one after making your working copy and that's all.

Even that, you can make those calcs inside your map instead like I did in the code above so you get the values cooked and ready to serve in the working copy of your array and with a single line! (or 3 if you use a lint :D )

As BONUS info, you "can't" set new values on a const as is (there are ways, ok, but let's work on standard with no hacks) so if you got a const with some data and you only need a part of it, you can create your cooked copy with map and use it whenever needed, the garbage collector should do the rest.

xtofl • Sep 27 '21

That's condescending. You probably meant 'they have a lot to learn'?

Yann MICHAUX • Sep 27 '21

Yeah, something like that

A.B. Samma • Sep 27 '21 • Edited

Great writeup! There's also some documentation at MDN under the Array.prototype.map() article on this antipattern (which is what it is).

Никола Шијакињић • Sep 28 '21

Dude, it is not redundant map, it is immutable one. There are reasons why people are using it.

Han Mai • Oct 2 '21

Don't worry, it will make your code smelly only. Memory will not be an issue.

David Alberto Adler • Sep 29 '21

This kind of micro-optimization is the devil's dust. Premature optimizations like this often lead to ugly code. Tools not rules!

VinayakSinghCS • Jan 2

Nice article will make sure to use forEach instead of map wherever necessary

Mikaleb • Oct 25 '21

Yeah I know, but Object.keys, Object.whatever with different results with unknown objects (if no ts)

Mikaleb • Sep 30 '21

The day we have map() working for objects it'll be everywhere :')

Nate • May 20

I guess your code reviews do not involve React developers. In React, Map is the only way to replace items in an array without mutating props.
react.dev/learn/updating-arrays-in...