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)

Answer 1

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.