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. — , Sep 11 '17 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, Sep 11 '17 at 22:44
That’s probably because you mutate the array as you iterate it. Just use `reduce` as in [this answer](https://stackoverflow.com/a/5669730/4642212). — Sebastian Simon, Sep 11 '17 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, Sep 11 '17 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, Sep 11 '17 at 22:54
"_Can someone please explain why it is a bad idea to mutate an array while iterating it_" Because it causes issues like the one you are facing right now... — takendarkk, Sep 11 '17 at 23:14

Gunjan Gupta · Accepted Answer · 2017-09-11T22:46:31.887

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 '17 at 22:46

answered Sep 11 '17 at 22:44

Gunjan Gupta

131
7

1

Rule of thumb, never iterate over array using forEach if you are going to change it – Gunjan Gupta Sep 11 '17 at 22:45
This is a comment, not an answer. – Sep 11 '17 at 22:46
3

it's a good enough answer for the question "What's wrong with this JavaScript Code (Arrays)?" – Hugo Silva Sep 11 '17 at 22:47
1

He asked for whats worng, I am telling him whats wrong. As far as I can see thats an answer – Gunjan Gupta Sep 11 '17 at 22:47

score 5 · Answer 2 · answered Sep 11 '17 at 22:45

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 '17 at 22:45

baao

71,625
17
143
203

As a supplement, there is a pretty good write up on map/reduce operations here: https://danmartensen.svbtle.com/javascripts-map-reduce-and-filter – AJ X. Sep 11 '17 at 22:50

score 0 · Answer 3 · answered Sep 11 '17 at 23:03

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

score 0 · Answer 4 · answered Sep 11 '17 at 23:10

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"].

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

4 Answers4