What's wrong with this JavaScript Code (Arrays)?

Question

Below is a code snippet which finds out "How many times each element of an array is repeated". Everything is working as expected except it doesn't give me "ebay repetition count"

var strArr = ["google","ebay","yahoo","google","ebay","facebook","facebook","google"];
var output= {};
strArr.forEach(function(element) {
    var count = 0;
    try{
        while(strArr.indexOf(element) !== -1){
            strArr.splice(strArr.indexOf(element),1);
            count++;

        }  
    }catch(e){
        console.log(e);
    }
 output[element] = count;
});
console.log(output);

I tried debugging it with debugger;, I figured out that the second element of the array is being skipped.

Expected Output:

google:3
ebay:2
yahoo:1
facebook:2

Actual Output:

google:3
yahoo:1
facebook:2

Looks like you're changing an array while iterating over it. That's a really bad idea. — user5734311
– user5734311, Commented Sep 11, 2017 at 22:43
You are modifying the array (with .splice()) in the middle of a forEach loop. This is causing some elements to be skipped, because the indices that existed when the forEach began are unrelated to the ones that exist now. — Nicholas Tower
– Nicholas Tower, Commented Sep 11, 2017 at 22:44
That’s probably because you mutate the array as you iterate it. Just use reduce as in this answer. — Sebastian Simon
– Sebastian Simon, Commented Sep 11, 2017 at 22:44
Can someone please explain why it is a bad idea to mutate an array while iterating it, and what is causing this issue in this case? I agree with everyone saying reduce is a best practice but I am not exactly looking for a best practice here. I am just trying to write simple javascript. — user3423927
– user3423927, Commented Sep 11, 2017 at 22:51
I personally wound't say it's a bad idea, you can iterate and change arrays at the same time, but you have to be aware of these gotchas. If you do want to mutate while iterating, you could have used a reverse for loop, removing items then wouldn't change the index positions. — Keith
– Keith, Commented Sep 11, 2017 at 22:54

Gunjan Gupta · Accepted Answer · 2017-09-11 22:46:31Z

5

ForEach take google from index 0:
you removed google from the list including zero
ForEach moves to index 1 which now has yahoo as ebay moved to index 0

Sorry for bad english

edited Sep 11, 2017 at 22:46

answered Sep 11, 2017 at 22:44

Gunjan Gupta

1317 bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

Gunjan Gupta Over a year ago

Rule of thumb, never iterate over array using forEach if you are going to change it

user5734311 Over a year ago

This is a comment, not an answer.

Hugo Silva Over a year ago

it's a good enough answer for the question "What's wrong with this JavaScript Code (Arrays)?"

Gunjan Gupta Over a year ago

He asked for whats worng, I am telling him whats wrong. As far as I can see thats an answer

baao · Accepted Answer · 2017-09-11 22:45:10Z

5

First you shouldn't change an array while iterating over it (as ChrisG mentioned in a comment), second there's really no need to loop in a loop. Use reduce instead

let strArr = ["google", "ebay", "yahoo", "google", "ebay", "facebook", "facebook", "google"];
let res = strArr.reduce(function(a, b) {
  a[b] = (a[b] || 0) + 1;
  return a;
}, {});

console.log(res);

answered Sep 11, 2017 at 22:45

baao

73.5k18 gold badges152 silver badges208 bronze badges

1 Comment

AJ X. Over a year ago

As a supplement, there is a pretty good write up on map/reduce operations here: danmartensen.svbtle.com/javascripts-map-reduce-and-filter

Joshua K · Accepted Answer · 2017-09-11 23:03:25Z

0

instead of using reduce, you can use a simple loop:

let strArr = ["google", "ebay", "yahoo", "google", "ebay", "facebook", "facebook", "google"];
let output = {};
strArr.forEach(function(val) {
  output[val] = output[val] || 0;
  output[val]++;
});

console.log(output);

answered Sep 11, 2017 at 23:03

Joshua K

2,5371 gold badge12 silver badges15 bronze badges

Comments

Slai · Accepted Answer · 2017-09-11 23:10:33Z

0

You can easily notice the issue by logging the result in the loop:

var strArr = ["google","ebay","yahoo","google","ebay","facebook","facebook","google"];
var output= {};
strArr.forEach(function(element) {
    var count = 0;
    while(strArr.indexOf(element) !== -1){
        strArr.splice(strArr.indexOf(element),1);
        count++;
        console.log(element, count, JSON.stringify(strArr)); // <-- added this line
    }  
    output[element] = count;
});
console.log(output);

As you can see, after the first item "google" is removed, the second item is not ebay, but yahoo! After yahoo is removed, the next item is not ebay, but facebook, and the array is left with ["ebay","ebay"].

answered Sep 11, 2017 at 23:10

Slai

23k5 gold badges49 silver badges55 bronze badges

Collectives™ on Stack Overflow

What's wrong with this JavaScript Code (Arrays)?

4 Answers 4

4 Comments

1 Comment

Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

4 Comments

1 Comment

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related