JavaScript - Optimizing a two array.map loop [closed]

The name of the pictureThe name of the pictureThe name of the pictureClash Royale CLAN TAG#URR8PPP





.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;







up vote
2
down vote

favorite












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 ?







share|improve this question













closed as off-topic by Graipher, Sam Onela, t3chb0t, Mast, Vogel612♦ Jan 15 at 15:02


This question appears to be off-topic. The users who voted to close gave this specific reason:


  • "Questions must involve real code that you own or maintain. Pseudocode, hypothetical code, or stub code should be replaced by a concrete implementation. Questions seeking an explanation of someone else's code are also off-topic." – Graipher, Sam Onela, t3chb0t
If this question can be reworded to fit the rules in the help center, please edit the question.












  • 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.
    – Marc Rohloff
    Jan 12 at 20:08






  • 1




    please give us samples of the two arrays, would be helpful
    – JohnPan
    Jan 13 at 16:02










  • Without knowing what the data looks like, we can't tell you what the best solution would look like.
    – Mast
    Jan 14 at 20:33
















up vote
2
down vote

favorite












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 ?







share|improve this question













closed as off-topic by Graipher, Sam Onela, t3chb0t, Mast, Vogel612♦ Jan 15 at 15:02


This question appears to be off-topic. The users who voted to close gave this specific reason:


  • "Questions must involve real code that you own or maintain. Pseudocode, hypothetical code, or stub code should be replaced by a concrete implementation. Questions seeking an explanation of someone else's code are also off-topic." – Graipher, Sam Onela, t3chb0t
If this question can be reworded to fit the rules in the help center, please edit the question.












  • 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.
    – Marc Rohloff
    Jan 12 at 20:08






  • 1




    please give us samples of the two arrays, would be helpful
    – JohnPan
    Jan 13 at 16:02










  • Without knowing what the data looks like, we can't tell you what the best solution would look like.
    – Mast
    Jan 14 at 20:33












up vote
2
down vote

favorite









up vote
2
down vote

favorite











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 ?







share|improve this question













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 ?









share|improve this question












share|improve this question




share|improve this question








edited Jan 12 at 12:28
























asked Jan 12 at 12:22









Mouad Ennaciri

486




486




closed as off-topic by Graipher, Sam Onela, t3chb0t, Mast, Vogel612♦ Jan 15 at 15:02


This question appears to be off-topic. The users who voted to close gave this specific reason:


  • "Questions must involve real code that you own or maintain. Pseudocode, hypothetical code, or stub code should be replaced by a concrete implementation. Questions seeking an explanation of someone else's code are also off-topic." – Graipher, Sam Onela, t3chb0t
If this question can be reworded to fit the rules in the help center, please edit the question.




closed as off-topic by Graipher, Sam Onela, t3chb0t, Mast, Vogel612♦ Jan 15 at 15:02


This question appears to be off-topic. The users who voted to close gave this specific reason:


  • "Questions must involve real code that you own or maintain. Pseudocode, hypothetical code, or stub code should be replaced by a concrete implementation. Questions seeking an explanation of someone else's code are also off-topic." – Graipher, Sam Onela, t3chb0t
If this question can be reworded to fit the rules in the help center, please edit the question.











  • 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.
    – Marc Rohloff
    Jan 12 at 20:08






  • 1




    please give us samples of the two arrays, would be helpful
    – JohnPan
    Jan 13 at 16:02










  • Without knowing what the data looks like, we can't tell you what the best solution would look like.
    – Mast
    Jan 14 at 20:33
















  • 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.
    – Marc Rohloff
    Jan 12 at 20:08






  • 1




    please give us samples of the two arrays, would be helpful
    – JohnPan
    Jan 13 at 16:02










  • Without knowing what the data looks like, we can't tell you what the best solution would look like.
    – Mast
    Jan 14 at 20:33















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.
– Marc Rohloff
Jan 12 at 20:08




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.
– Marc Rohloff
Jan 12 at 20:08




1




1




please give us samples of the two arrays, would be helpful
– JohnPan
Jan 13 at 16:02




please give us samples of the two arrays, would be helpful
– JohnPan
Jan 13 at 16:02












Without knowing what the data looks like, we can't tell you what the best solution would look like.
– Mast
Jan 14 at 20:33




Without knowing what the data looks like, we can't tell you what the best solution would look like.
– Mast
Jan 14 at 20:33










3 Answers
3






active

oldest

votes

















up vote
1
down vote



accepted










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;







share|improve this answer




























    up vote
    2
    down vote













    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.






    share|improve this answer




























      up vote
      0
      down vote













      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];

      );





      share|improve this answer





















      • Is that not a little unsafe if Object`s prototype has been modified?
        – Blindman67
        Jan 12 at 23:15










      • Modified in what way? So that lookups aren't O(1)?
        – Barmar
        Jan 12 at 23:22










      • 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.
        – Blindman67
        Jan 12 at 23:42

















      3 Answers
      3






      active

      oldest

      votes








      3 Answers
      3






      active

      oldest

      votes









      active

      oldest

      votes






      active

      oldest

      votes








      up vote
      1
      down vote



      accepted










      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;







      share|improve this answer

























        up vote
        1
        down vote



        accepted










        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;







        share|improve this answer























          up vote
          1
          down vote



          accepted







          up vote
          1
          down vote



          accepted






          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;







          share|improve this answer













          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;








          share|improve this answer













          share|improve this answer



          share|improve this answer











          answered Jan 12 at 23:11









          Blindman67

          5,4001320




          5,4001320






















              up vote
              2
              down vote













              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.






              share|improve this answer

























                up vote
                2
                down vote













                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.






                share|improve this answer























                  up vote
                  2
                  down vote










                  up vote
                  2
                  down vote









                  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.






                  share|improve this answer













                  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.







                  share|improve this answer













                  share|improve this answer



                  share|improve this answer











                  answered Jan 12 at 17:58









                  A Bravo Dev

                  539110




                  539110




















                      up vote
                      0
                      down vote













                      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];

                      );





                      share|improve this answer





















                      • Is that not a little unsafe if Object`s prototype has been modified?
                        – Blindman67
                        Jan 12 at 23:15










                      • Modified in what way? So that lookups aren't O(1)?
                        – Barmar
                        Jan 12 at 23:22










                      • 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.
                        – Blindman67
                        Jan 12 at 23:42














                      up vote
                      0
                      down vote













                      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];

                      );





                      share|improve this answer





















                      • Is that not a little unsafe if Object`s prototype has been modified?
                        – Blindman67
                        Jan 12 at 23:15










                      • Modified in what way? So that lookups aren't O(1)?
                        – Barmar
                        Jan 12 at 23:22










                      • 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.
                        – Blindman67
                        Jan 12 at 23:42












                      up vote
                      0
                      down vote










                      up vote
                      0
                      down vote









                      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];

                      );





                      share|improve this answer













                      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];

                      );






                      share|improve this answer













                      share|improve this answer



                      share|improve this answer











                      answered Jan 12 at 22:58









                      Barmar

                      27019




                      27019











                      • Is that not a little unsafe if Object`s prototype has been modified?
                        – Blindman67
                        Jan 12 at 23:15










                      • Modified in what way? So that lookups aren't O(1)?
                        – Barmar
                        Jan 12 at 23:22










                      • 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.
                        – Blindman67
                        Jan 12 at 23:42
















                      • Is that not a little unsafe if Object`s prototype has been modified?
                        – Blindman67
                        Jan 12 at 23:15










                      • Modified in what way? So that lookups aren't O(1)?
                        – Barmar
                        Jan 12 at 23:22










                      • 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.
                        – Blindman67
                        Jan 12 at 23:42















                      Is that not a little unsafe if Object`s prototype has been modified?
                      – Blindman67
                      Jan 12 at 23:15




                      Is that not a little unsafe if Object`s prototype has been modified?
                      – Blindman67
                      Jan 12 at 23:15












                      Modified in what way? So that lookups aren't O(1)?
                      – Barmar
                      Jan 12 at 23:22




                      Modified in what way? So that lookups aren't O(1)?
                      – Barmar
                      Jan 12 at 23:22












                      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.
                      – Blindman67
                      Jan 12 at 23:42




                      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.
                      – Blindman67
                      Jan 12 at 23:42


                      Popular posts from this blog

                      Chat program with C++ and SFML

                      Function to Return a JSON Like Objects Using VBA Collections and Arrays

                      Will my employers contract hold up in court?