DEV Community

Cover image for Stop abusing .map()!

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', ...
Collapse
totally_chase profile image
Phantz

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.

Collapse
pitops profile image
Petros Kyriakou

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

Collapse
totally_chase profile image
Phantz • Edited on

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]];
  }
}
Enter fullscreen mode Exit fullscreen mode

playground

Thread Thread
pitops profile image
Petros Kyriakou

yes i thought you were talking about Object.entries

Collapse
davidmaxwaterman profile image
Max Waterman

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;
  }
}
Enter fullscreen mode Exit fullscreen mode

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

Collapse
shanoaice profile image
Andy Chen

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......

Thread Thread
ashleyjsheridan profile image
Ashley Sheridan

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?

Collapse
bugenzi profile image
Amar Bugarin

Can you elaborate on the performance part?

Collapse
lethanhtupk profile image
Tu Le Thanh • Edited on

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

Collapse
totally_chase profile image
Phantz

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.

Collapse
tanth1993 profile image
tanth1993

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

Collapse
joelbonetr profile image
JoelBonetR • Edited on

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>;
    })}
  </>);
Enter fullscreen mode Exit fullscreen mode

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 😆

Collapse
lmorchard profile image
Les Orchard • Edited on

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.

Collapse
joelbonetr profile image
JoelBonetR • Edited on

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.

Thread Thread
owenmelbz profile image
Owen Melbourne

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

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

Maybe a language barrier thing.

Collapse
momander profile image
Martin Omander

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!

Collapse
lionelrowe profile image
lionel-rowe

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.

Collapse
xtofl profile image
xtofl

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?

Collapse
totally_chase profile image
Phantz

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.

Collapse
antonioaltamura profile image
antonioaltamura • Edited on

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.

Collapse
darrylhodgins profile image
Darryl Hodgins

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')
);
Enter fullscreen mode Exit fullscreen mode
Collapse
davidmaxwaterman profile image
Max Waterman

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');
});
Enter fullscreen mode Exit fullscreen mode

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.

Collapse
pitops profile image
Petros Kyriakou

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.

Collapse
fawazsullia profile image
fawazsullia

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.

Collapse
brianmcbride profile image
Brian McBride

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).

Collapse
davidmaxwaterman profile image
Max Waterman

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.

Collapse
hyungjunk profile image
hyungjunk • Edited on

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?

Collapse
ccampanale profile image
Christopher Campanale

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

😘

Collapse
code913 profile image
code913

I heard forEach is slower than for...of is this true?

Collapse
dimaboychev profile image
Dmitry Boichev

Probably but it’s negligible

Collapse
larsonnn profile image
Lars Feldeisen • Edited on

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

Collapse
mikaleb profile image
Mikaleb

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

Collapse
lukeshiru profile image
LUKESHIRU

I mean:

Object.entries(object).map(([key, value]) => /* Do whatever you want here */)
Enter fullscreen mode Exit fullscreen mode
Collapse
mikaleb_77 profile image
Mikaleb

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

Collapse
abesamma profile image
A.B. Samma • Edited on

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

Collapse
lewiscowles1986 profile image
Lewis Cowles

😑 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.

Collapse
jessycormier profile image
Jessy Cormier

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.

Collapse
marcosborunda profile image
Marcos Borunda

😑 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.

Collapse
ugizashinje profile image
Никола Шијакињић

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

Collapse
youdev profile image
Younes

There is also a performance cost to consider when looping:

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

Collapse
pranaythesingh profile image
Pranay Singh

Nice Post

Collapse
tanth1993 profile image
tanth1993

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

Collapse
mfbx9da4 profile image
David Alberto Adler

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

Collapse
maixuanhan profile image
Han Mai

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

Collapse
yannmcx profile image
Yann MICHAUX

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

Collapse
joelbonetr profile image
JoelBonetR • Edited on

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',
  ...
}, {
  ...
}]
Enter fullscreen mode Exit fullscreen mode

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>
Enter fullscreen mode Exit fullscreen mode

so you get

<span id="1"> J. Doe </span>
Enter fullscreen mode Exit fullscreen mode

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.

Collapse
xtofl profile image
xtofl

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

Collapse
yannmcx profile image
Yann MICHAUX

Yeah, something like that