5
\$\begingroup\$

I have to iterate two arrays, if the iso propery of the first array is equal to address.country of the second array condition is verified, assign address and slug of the second array (this.concessions) to the first array (this.countries).

at the end, you need to have a new this.countries array that contains theaddress and slug property (in addition to the properties he already had)

this.countries.map((element) => {
  this.concessions.map((value) => {
    if (element.iso === value.address.country) {
      element.address = value.address
      element.slug = value.slug
    }
  })
})

How can I optimize this, and for this case what is the best iterable to use, for ..of for example ?

\$\endgroup\$
  • \$\begingroup\$ If you aren't using the result of map you should really be using forEach or a plain for loop (as per A Bravo Dev`s second example. \$\endgroup\$ – Marc Rohloff Jan 12 '18 at 20:08
  • 1
    \$\begingroup\$ please give us samples of the two arrays, would be helpful \$\endgroup\$ – JohnPan Jan 13 '18 at 16:02
  • \$\begingroup\$ Without knowing what the data looks like, we can't tell you what the best solution would look like. \$\endgroup\$ – Mast Jan 14 '18 at 20:33
2
\$\begingroup\$

For performance use a for ... of loop, it has much less overhead as it does not require a new context and heap allocation for each iteration that are required when you use the array iteration methods like Array.forEach.

Also replacing the array using Array.map requires two copies of the array to exist at the same time, the new array being generated by Array.map and the existing array, on top of the heap allocations that Array.map requires.

So the most efficient way is

for(const country of this.countries) {
    for(const conc of concessions){
       if(conc.address.country === country.iso){
          country.address = conc.address;
          country.slug = conc.slug;
          break; // only set first occurrence
       }
    }
}

You could also use a Map to lookup the iso. Though a little more memory usage it will be somewhat quicker overall if you maintained the concessions iso map alongside the concessions array.

// The map needs to be created.
// but ideally you would maintain the map alongside code that maintains concessions
const concessionsIsoMap = new Map();
for(const conc of concessions) { 
     concessionsIsoMap.set(conc.address.country, conc);
}
for(const country of this.countries) {
   // lookup for concessions.address.country is very fast as it uses
   // a hash table to find the correct item, if any.
   const concession = concessionsIsoMap.get(country.iso);
   if(concession){
       country.address = conc.address;
       country.slug = conc.slug;
   }
}
| improve this answer | |
\$\endgroup\$
3
\$\begingroup\$

That'd be with map:

this.countries = this.countries.map(country => {
    var concession = concessions.find(x => x.address.country === country.iso);
    country.address = concession.address;
    country.slug = concession.slug;
    return country;
});

That'd be with forEach:

this.countries.forEach(country => {
    var concession = concessions.find(x => x.address.country === country.iso);
    country.address = concession.address;
    country.slug = concession.slug;
});

I don't know if you can be sure there always be a concession for each country and how you should proceed in case there isn't.

| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

Your algorithm is O(n2) because you're performing a linear search of concessions for each element in countries. If these arrays are large, it's better to create an object that maps countries directly to concessions. Object lookups are O(1), so the whole algorithm becomes O(n).

const countryMap = {};
this.concessions.forEach(c => countryMap[c.address.country] = c);

this.countries.forEach(country => {
    var concession = countryMap[country.iso];
    if (concession) {
        [country.address, country.slug] = [concession.address, concession.slug];
    }
});
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Is that not a little unsafe if Object`s prototype has been modified? \$\endgroup\$ – Blindman67 Jan 12 '18 at 23:15
  • \$\begingroup\$ Modified in what way? So that lookups aren't O(1)? \$\endgroup\$ – Barmar Jan 12 '18 at 23:22
  • \$\begingroup\$ So there is no key clash. You should create a object used as a map via const countryMap = Object.create(null); to remove any possible key clash. \$\endgroup\$ – Blindman67 Jan 12 '18 at 23:42

Not the answer you're looking for? Browse other questions tagged or ask your own question.