Javascript: forEach() loop to populate an array - closure issue

Question

Let's say we have an array of objects like:

var fruits = [ {name:"banana", weight:150},{name:"apple", weight:95},{name:"orange", weight:160},{name:"kiwi", weight:80} ];

I want to populate a "heavy_fruits" array with items from the "fruits" array above which weight is > 100. Here is my code:

var heavy_fruits = [];
myfruit = {};

fruits.forEach(function(item,index) {
  if ( item.weight > 100 ) { 
    myfruit ["name"] = item.name;
    myfruit ["weight"] = item.weight;
  }

heavy_fruits.push(myfruit);
});

However it shows: name:"orange", weight:160 name:"orange", weight:160 name:"orange", weight:160 name:"orange", weight:160

I know this is an issue with mixing closures and loops... but I read an article (http://zsoltfabok.com/blog/2012/08/javascript-foreach/) explaining that I would avoid this kind of issue using a forEach loop instead of the classic for loop.

I know I can use array methods like filter(), etc. but I'm asking that on purpose since I'm actually having troubles with a much bigger function that I cannot expose here... So I tried to summarize and simplify my issue description with "fruits".

Solution 1

var heavy_fruits = [];
myfruit = {}; // here's your object

fruits.forEach(function(item,index) {
    if ( item.weight > 100 ) { 
        myfruit ["name"] = item.name;
        myfruit ["weight"] = item.weight; // you modify it's properties
    }

    heavy_fruits.push(myfruit); // you push it to the array
});

You end up with an array [myfruit, myfruit, myfruit, myfruit].

Now if you modify myfruit anywhere in the code, the change will be visible in every single occurence of myfruit. Why?

Because you are modifying the referenece to the object. In this example, your array stores just copies of your object. And if you change one of it, every single one will change, because they are all references.

To fix this with each iteration you should be creating a new object and then doing some stuff on it.

BTW, as a matter of fact, your if could just be like this:

if ( item.weight > 100 ) { 
    heavy_fruits.push(item); // if `item` only has `name` and `weight` properties
}

Solution 2

 fruits.forEach(function (item, index) {
  if (item.weight > 100) {
    myfruit = {};
    myfruit["name"] = item.name;
    myfruit["weight"] = item.weight;
    heavy_fruits.push(myfruit);
  }
}); 

Comments

[Tushar]

myfruit is referencing to same object. Move myfruit = {}; in the forEach callback. And I'll suggest to use filter as var heavy_fruits = fruits.filter(f => f.weight > 100);.

[Tushar]

It's not closure issue, it's about referencing same object.

[Admin]

Basically what you are doing is mutating object's properties and because the array is in fact storing references to the same object, when you change one of the object's property, the change is visible in each reference.

[Redu]

Amongst all array methods forEach has got <= 2% use case. For this particular case you should use either filter or reduce.

[Kate]

I was having the same issue as the original poster, and the thing that fixed it for me was the first comment above by Tushar. The variable where the empty object is declared must be inside the forEach.

[Tushar]

Please edit with more information. Code-only and "try this" answers are discouraged, because they contain no searchable content, and don't explain why someone should "try this". We make an effort here to be a resource for knowledge.

[Tushar]

This is not different from the code in question. Except, this'll give array of two oranges than four.

[Gangz]

heavy_fruits.push(myfruit); was out of the if scope ..that is the problem

[Gangz]

That is a code problem ..I don't find any other issues with that

[nadir]

Thank you so much to everyone who took time to help me to find the solution: Albzi, Tushar, Adrian and Ganga. However Adrian provided the solution + the clear explanation of my issue + why it does not work + how I can fix it. I've struggling with that issue for days! Regards