Is performing a mapping operation without using returned value an antipattern?

Question

Lets say I have a list and I want to add some values into it using a mapping function:

const arr = [1, 2, 3, 4, 5];
const anotherArr = [];

I do this using a functional approach:

arr.map((item) => anotherArr.push(item));

Is this an antipattern / bad logic - that is, not using the mapping operation return value for anything? Are there any good resources on this?

(I know this logic is silly and I can just copy the list - that is not the point of my question)

What's the point of using `map` over `foreach` here? Also I wouldn't really consider this a functional approach since it involves mutating an array. — sepp2k, Jul 05 '19 at 13:10
There is no point really except providing some code to my question. I was hoping it would not be necessary to draw up anything more complicated than this to illustrate my point. If not, then please disregard all code provided and please answer the actual question (post heading) — Flimzy_Programmer, Jul 05 '19 at 13:15
This is an anti-pattern - you are completely misusing the `.map` method. What you *might* use instead is `.forEach` which will at least have the correct semantics for your operation, whereas `.map` is very misleading tow hat is happening. — VLAZ, Jul 05 '19 at 13:33
Yes it is an anti pattern because people would assume `const xs = arr.map((item) => anotherArr.push(item)` to be a valid operation, the resulting type `[undefined]` isn't valid though. — , Jul 05 '19 at 13:33
@melbil I get that your particular code is just an example, but the general situation you described is "not using the mapping operation return value for anything" and I'm asking why you would use `map` over `foreach` in any situation that fits that description - not just this specific example. My question applies equally to any more complicated example that you might draw up. — sepp2k, Jul 05 '19 at 14:08
`map` builds an array - if you're going to just throw it away, what's the point of wasting the memory and time to build it? Yes this is an anti-pattern. `forEach` is intended for side-effects, eg `push`, so just use `forEach`. — Mulan, Jul 05 '19 at 20:07

score 39 · Accepted Answer · edited Oct 06 '21 at 15:46

Yes, this is an anti-pattern. Although you could argue it is not and it is just plain misuse. I would call it an anti-pattern because for some reason there is a frequent widespread incidents of people using .map() when they should not.

The term comes from mathematics where you can map from one category to another. For example, shapes (X) to colours (Y):

(image from Wikipedia)

The term is also well established in computer science where map() is a higher order function doing this sort of conversion. In JavaScript, it is an array method and has clear usage - to transform the contents of one array into another. Given array X = [0, 5, 8, 3, 2, 1] we can apply x => x + 1 to it using the .map() method.

(Image from Wikipedia)

This is more wide-reaching than just the specifics of the implementation - .map() is idiomatic and if misused makes code harder to read and understand. Let's do a step-by step example:

We need a mapping function that expresses the relationship between elements. For example, transforming a letter to its position in the alphabet can be expressed via the function:

function letterToPositionInAlphabet(letter) {
  return letter.toUpperCase().charCodeAt(0) - 64;
}

Mapping an array of letters via this function will give us an array with each of their positions:

function letterToPositionInAlphabet(letter) {
  return letter.toUpperCase().charCodeAt(0) - 64;
}

const letters = ["a", "b", "c"];

console.log(letters.map(letterToPositionInAlphabet));

The mapping operation is an idiom and part of understanding the code. If you see someArr.map(someFn) it sets up expectations and it is easy to understand what sort of operation is happening, without needing to know the contents of either the array or the function. When you see letters.map(letterToPositionInAlphabet) it should be trivial to understand what the intent is - get the positions in the alphabet of some letters. This is self-documenting code, we can assume the code is correct unless proven otherwise.

However, using .map() as .forEach() is breaking that intended meaning and can be confusing to read. Consider this

function playerToPlaceInRankList(player) {
   const position = lookupPlayerRank(player);
   positionsArr.push(position);
}

/* many lines later */

players.map(playerToPlaceInRankList);

/* more code */

The line which seems like it performs mapping also immediately looks wrong because the return value is ignored. Either that line is not needed, or you have to examine what playerToPlaceInRankList does in order to find out what is actually happening here. That is unnecessary mental load for just reading what should be a straight-forward and self-documenting line of code.

The same applies to using other methods like .filter(), .find(), .every(), .some(), etc. Do not use those just because they iterate over the array, if what you want is not what they are intended to do.

Is performing a mapping operation without using returned value an antipattern?

1 Answers1

Linked