Pan Seba

Posted on Sep 26, 2021

Stop abusing .map()!

#javascript #programming #functional #codereview

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', 'oragne', 'banana'];
fruitIds.map((id) => {
   document.getElementById(`fruit-${id}`).classList.add('active');
});

So as you can see it's just a simple iteration where for every element in the fruitIds array we add active class to a certain HTML element in a DOM.

Many programmers (especially new) wouldn't notice anything wrong with the code above. However, there is a one major issue here - the usage of .map(). Let me explain.

What is wrong with `.map()`?

Well, there is completely nothing wrong with this particular array method. In fact, I think it is very handy and beautifully wraps one of the iteration patterns - mapping.

In simple words, mapping is an operation which applies a function to every element of a collection and returns a new collection with elements changed by the mentioned function. For example, if we have an array of numbers const nums = [1, 2, 3, 4]; and would like to receive a new array of doubled numbers, we could map the original array to a new one like this (in JavaScript):

const biggerNums = nums.map((n) => n * 2);
// >> [2, 4, 6, 8];

The biggerNums array would consist of numbers from the original nums array multiplied by 2.

Notice how .map() is used - we assigned the result of this method to a new variable called biggerNums. I have also mentioned earlier that mapping is an operation that returns a new collection of elements. And this is the very reason the code snippet showed at the beginning of this article is wrong. The .map() returns a new array - always - and if we don't need that array, we shouldn't use .map() in the first place. In this particular case (simple iteration) a different array method should be used - .forEach() - which is specifically designed for such cases. It doesn't return a new collection, it simply walks through an array and invokes a callback function for every element allowing you to do something for each of them.

So the correct version of the mentioned snippet should look like this:

// good way
const fruitIds = ['apple', 'oragne', 'banana'];
fruitIds.forEach((id) => {
   document.getElementById(`fruit-${id}`).classList.add('active');
});

We don't need a new array so we simply iterate over the fruitIds array and add the active class to an HTML element for each of the array items.

Okay, but why should I care? .map() is shorter and easier to write than .forEach(). What could possibly go wrong?

Consequences of abusing `.map()`

One of the worst consequences of abusing .map() is the fact that it returns a new redundant array. To be more specific - it returns a new array of the same size as the one this method was called on. It means that if we have an array of 1000 elements, .map() will return a new array of 1000 elements - every time.

In JavaScript, all functions return a value. Even if we don't use the return keyword, the function will return undefined implicitly. That's how the language has been designed. This rule also applies to callbacks - they are functions too.

Having said that, let's get back to the original example:

// wrong way
const fruitIds = ['apple', 'oragne', 'banana'];
fruitIds.map((id) => {
   document.getElementById(`fruit-${id}`).classList.add('active');
});

What happens here? An array of fruit IDs is created and then it's mapped to another array of the same size. Even though the array returned by .map() is not used, it does take place in memory. This new (unused) array looks like this:

[undefined, undefined, undefined]

It's because the callback passed to the .map() method does not have the return keyword and as we know, if there is no return, undefined is returned implicitly.

How bad is it? Very bad. In this particular example it won't bring any serious consequences - there are only three items in the array so creating another three-element array won't cause any problems. However, the problem arises when we deal with big arrays of complex data. If we want to iterate over an array of five thousand objects and we abuse .map(), we create another array of five thousand elements - undefineds. So we end up storing 10 000 elements in memory from which a whole half is redundant. It is a very non-optimal practice and in some scenarios it may even lead to the application overload. This is why we should pick right methods to the right tasks.

Summary

There are many practices that are essentialy bad, but the negative consequences will start to be visible only when dealing with bigger datasets. One of such practices is abuse of .map(). When operating on small arrays, it won't cause any hurt. But when we make this mistake with a bigger array it will start overloading our application and it may be quite hard to debug.

This is why we should never let it pass by and whenever we see this abuse, we should take care of it. I hope now you understand why.

Top comments (48)

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.