Group inner properties from an array of objects to a map

Question

I have an array of object and I want to group/order some inner properties in a map according to a key. I did this code but I would like to know if there was a way to optimize the code ?

const myArray = [{
    getValue: () => {
      return {
        country: 'France',
        city: 'Toulouse',
      };
    },
  },
  {
    getValue: () => {
      return {
        country: 'England',
        city: 'London',
      };
    },
  },
  {
    getValue: () => {
      return {
        country: 'France',
        city: 'Paris',
      };
    },
  },
];
const myMap = new Map();
myArray.forEach(item => {
  const key = item.getValue().country;
  const value = item.getValue().city;
  if (myMap.has(key)) {
    myMap.get(key).push(value);
  } else {
    myMap.set(key, [value]);
  }
});


for (const [key, value] of myMap) {
  console.log(`${key} = ${value}`);
}

Answer 1

Too complex

Don't make code more complicated than needed.

The getValue function for each data item is entirely unnecessary. You can move the function out and reduce the data input complexity a great deal. See 1st rewrite.

For each data item property you call getValue (once for country and once for city) which creates a new object each time it is called. This is a huge overhead, the items should be created only once (When code is parsed) and to process you should not need to make copies just to read properties.

If you need the data items to be functions, don't write the function for each item, write the function once and return it using a second function. See 2nd rewrite.

Template_literals can sometimes add a lot of noise to the construction of a string. Maybe ${key} = ${value} can be better as key + " = " + value. Template literals do not make for better code just by using them.

Names

The naming is very poor. You should never get lazy with naming, even if its example code.

• key and value should be country and city
• myMap Yes we know its a map, but what does it represent. citesByCountry

Code should never stand alone. Always write code as a named function, even when its just example code. See rewrite.

General points

Always code in Strict_mode.

Prefer for...of loops over Array iterators when possible.

You do two map look-ups with Map.has and Map.get if an index exists. You can avoid one of the look-ups and use only Map.get. Check the result to determine if the map has the index you are looking for. See rewrite.

Learn to use the Conditional Operator ?, Nullish coalescing operator ?? and / or Optional chaining ?. to simplify statement combinations.

Rewrites

First without the overhead of the getValue call per item.

"use strict";
function dataTest() {
    const addCountry = (country, city) => ({country, city});
    const data = [
        addCountry("France", "Toulouse"),
        addCountry("England", "London"),
        addCountry("France", "Paris"),
    ];
    function joinByCountry(data) {
        const countries = new Map();
        for (const {country, city} of data) {
            countries.get(country)?.push(city) ?? countries.set(country, [city]);
        }
        return countries;
    }

    for (const [country, cites] of joinByCountry(data)) { 
         console.log(country + " = " + cites);
    }
}

If getValue is required then write the getValue function once rather than for each item.

This rewrite uses Conditional Operator ? to replace the if else statements.

"use strict";
function dataTest() {
    const addCountry = (country) => ({ getValue() { return country} });
    const data = [
        addCountry({country: "France", city: "Toulouse"}),
        addCountry({country: "England", city: "London"}),
        addCountry({country: "France", city: "Paris"}),
    ];
    function joinByCountry(data) {
        const countries = new Map();
        for (const location of data) {
            const {country, city} = location.getValue(); 
            const cites = countries.get(country);
            cites ? cites.push(city) : countries.set(country, [city]);
        }
        return countries;
    }

    for (const [country, cites] of joinByCountry(data)) { 
         console.log(country + " = " + cites);
    }
}

Answer 2

The code looks okay as is, but I would destructure the return value of the function rather than calling twice and also remove the brackets from the conditionals since they’re both one-liners

const myMap = new Map();
myArray.forEach(item => {
  const {key, value} = item.getValue();
  
  if (myMap.has(key)) myMap.get(key).push(value);
  else myMap.set(key, [value]);
});