0

I have a function that generates numbers within a range. I created a composite type like this:

var cowPosition = {
    x: 0,
    y: 0
};

I also created an array:

var positionsArray = [];

then, I proceed to iterate to fill the array with the composite type.

All of this is inside a function which returns the array.

Here's the function:

function generateCowPositions(numberOfCows){
    var positionsArray = [];
    var cowPosition = {
        x: 0,
        y: 0
    };
    var x,y;

    for (var i = 0; i < numberOfCows; i++) {

        x = randomPosition(0,5);
        y = randomPosition(0,5);

        x = x * 80;
        y = y * 80;

        cowPosition.x = x;
        cowPosition.y = y;
        positionsArray[i] = cowPosition;
    }
    return positionsArray;

}

When I run it, it fills the whole array with the last two generated coordinates.

  • 4
    You need to create a *new* cowPosition on each iteration, currently you're modifying the single one and putting it in every array spot. – rorschach Aug 02 '17 at 19:38
  • What you're creating is better described as a "JavaScript Object" or just "object" and not a "composite type". – Dai Aug 02 '17 at 19:39
  • Possible duplicate of [How do JavaScript closures work?](https://stackoverflow.com/questions/111102/how-do-javascript-closures-work) – Elan Hamburger Aug 02 '17 at 19:41
  • Where are defined "cantidadVacas" variable in your function? – Fernando Torres Aug 02 '17 at 19:42
  • this is nothing to do with closures. – James Aug 02 '17 at 19:42
  • cantidadVacas is a forgotten-to-translate-variable-to-english (numberOfCows) – James Aug 02 '17 at 19:42
  • Thank you so much @rorschach, doing that solved the issue. And now i understand why. – Gianfranco Verrocchi Aug 02 '17 at 19:42
  • @James he's assuming that JavaScript will copy `cowPosition` into the array. JavaScript closures say that it won't get copied, it'll just get a reference to it and at the end they'll all be the same. – Elan Hamburger Aug 02 '17 at 19:45
  • @ElanHamburger You're right he's assuming it will create a new copy of cowPosition, but it still has nothing to do with closures. – James Aug 02 '17 at 19:56
  • @James Yes, it does. A closure is the mapping of the lexical environment to the code. He's working under the assumption that the environment will be bound on every loop iteration so that at the end you have an array of what the environment looked like at each iteration. But, as we all know, JavaScript has the closure outside of the loop, so you only get the environment at the end. – Elan Hamburger Aug 02 '17 at 20:04
  • @ElanHamburger So you are saying that knowing about the lexical environment where generateCowPositions is defined will somehow help us? I disagree, it's just a question about inserting the same variable into an array over and over, and then modifying that variable, and being surprised that the array changes. If that's related to closures then I need to do some reading. – James Aug 02 '17 at 20:17
  • @James That's the textbook example of closures in JavaScript. The rookie mistake that everyone makes (myself included) when first learning JavaScript is assuming that the closure is bound at array insertion (which it is in most languages -- perhaps you've been working in JavaScript for too long). This is literally MDN's [Closures in Loops: A Common Mistake](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Closures#Creating_closures_in_loops_A_common_mistake) – Elan Hamburger Aug 02 '17 at 20:43
  • Dude it's not! that's to do with defining a function in a loop with inner code that depends on the current state of an iterator outside that function (ie, the classical "closure"). That is not happening here. Anyway rookie is pretty rude, I'm done! – James Aug 02 '17 at 20:46
  • @James Closures can also refer to the lexical scope of a loop. The mistake illustrated is that the programmer assumes the variable is evaluated from the environment of each iteration of the loop when the variable is actually evaluated from the environment of the function. I wasn't trying to call you a rookie -- apologies if I offended. – Elan Hamburger Aug 02 '17 at 21:54

5 Answers5

3

There is no "composite type" in JavaScript. What you are referring to is called an object.

The problem you are having is that objects are passed by reference, not by value. This means that if you store an object into a variable called a and modify it in some function, the value stored in a will be modified too.

What you need to do is:

function generateCowPositions(numberOfCows) {
  var positionsArray = [];
  // note: cowPosition object is not needed anymore, so I've removed it
  var x, y;

  for (var i = 0; i < cantidadVacas; i++) {

    x = randomPosition(0, 5);
    y = randomPosition(0, 5);

    x = x * 80;
    y = y * 80;

    // create a new object in every intration
    positionsArray[i] = {
      x: x,
      y: y,
    };

  }
  return positionsArray;

}
Marko Gresak
  • 7,950
  • 5
  • 40
  • 46
  • Performance could be improved if the anonymous `{ x, y }` object-type were promoted to a `prototype` and then each instance created through a constructor call instead of creating a new object on each iteration. – Dai Aug 02 '17 at 19:43
  • @Dai I agree, but we are talking about learning basic concepts of JavaScript. I don't think there is any need to work on performance at this point. And I would argue that even in large production apps, optimizations like this are not really needed, you rarely have code that would be noticeably faster from this kind of optimization. – Marko Gresak Aug 02 '17 at 19:47
0

Because there is only one instance of cowPosition in your code. So every iteration of your loop simply changes that one object, and at the end of the loop you simple keep the result of the last iteration.

yusif
  • 193
  • 9
0

Each index in the array is pointing to the exact same object, being cowPosition

I'm not sure what you are trying to accomplish 100%, but you should create a new object at each iteration. There isn't a need to initialize x and y in the object.

Simply:

function generateCowPositions(numberOfCows){
var positionsArray = [];
var cowPosition = {}
var x,y;

for (var i = 0; i < cantidadVacas; i++) {


x = randomPosition(0,5);
y = randomPosition(0,5);

x = x * 80;
y = y * 80;

cowPosition.x = x;
cowPosition.y = y;
positionsArray[i]= Object.assign({}, cowPosition);

}
return positionsArray;

}
ACalza
  • 96
  • 1
  • 5
  • Note that this is a shallow copy, meaning that only the base-level properties are copied. It's fine for this case, but you need to be careful with deeply nested objects, e.g. `{ a: { b: { c: 1 } } }` will not be copied properly, reference to `a.b.c` will still stay the same. – Marko Gresak Aug 02 '17 at 19:45
0

JavaScript is an object-oriented language where objects are passed by-reference, not by-value. I suspect you're thinking of C and C++ where struct/class values are by default copied in their entirety by the = operator. In JavaScript it's closer to C# and Java where object / non-primitive types have their references copied instead.

Dai
  • 141,631
  • 28
  • 261
  • 374
0

Objects in javascript are passed as references.

positionsArray[i] = cowPosition; sets positionsArray[i] to a reference to the cowPosition object.

After the loop, all array elements reference the same cowPosition object. Therefore, since each loop of the array changes the underlying cowPosition object, all the array elements appear to have the same x and y position.

A simple solution to this problem would be to shallow copy the cowPosition object inside the loop, such that each array element references a difference position object and changing the underlying cowPosition object has no effect on the shallow copies inside the positionsArray, like so:

positionsArray[i] = JSON.parse(JSON.stringify(cowPosition));

This works by converting the object to a string and back again using javascript's native JSON implementation.

Seb Morris
  • 380
  • 1
  • 8