Yes, this is an anti-pattern. Although you could argue it isn't and it's just plain misuse. I'd call it an anti-pattern because for some reason there is a frequent widespread incidents of people using .map
when they shouldn't.
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's an array method and has clear usage - you 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;
}
So mapping an array of letters via this function will give you 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)
sets up expectations and it's easy to understand what sort of operation is happening, without needing to known 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. Having 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's unnecessary mental load for just reading what should be a straight forward line and self-documenting line of code.
The same applies to using other methods like .filter
, .find
, .every
, .some
, etc. Don't use those just because they iterate over the array, if what you want is not what they are intended to do.
map
overforeach
here? Also I wouldn't really consider this a functional approach since it involves mutating an array. – sepp2k Jul 5 '19 at 13:10.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 5 '19 at 13:33const xs = arr.map((item) => anotherArr.push(item)
to be a valid operation, the resulting type[undefined]
isn't valid though. – scriptum Jul 5 '19 at 13:33map
overforeach
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 5 '19 at 14:08