1

I would like to know whats wrong with my code. As in the title descripted I would like to turn around the content of an array with a for loop to another array. I would like to use ES5 for this since I am not used to ES6+ yet.

Here is my code:

    var arrayA = ["h","e","l","l","o"];
    var arrayB = [];
     function copyArray(oldArray, newArray) {
       oldArray.forEach(function() {
            var storeElement = oldArray.pop();
           newArray.push(storeElement); 
     });
       console.log(oldArray + " old array");
       console.log(newArray + " new array");
    }
    
    
    copyArray(arrayA, arrayB);

The result is:

"h,e,l,l old array"
"o new array"
"h,e,l old array"
"o,l new array"
"h,e old array"
"o,l,l new array"
"h,e FINAL old array"
"o,l,l FINAL new array"

But it should be:

"" FINAL old array
"o, l, l, e, h" FINAL new array. 

What is going wrong?

mkrieger1
  • 19,194
  • 5
  • 54
  • 65
Burg
  • 191
  • 3
  • 15
  • 6
    How about `let ary = ["h","e","l","l","o"].reverse();`? – Scott Marcus Jun 01 '18 at 16:19
  • Why not just use [**.reverse()**](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/reverse) – Nope Jun 01 '18 at 16:19
  • 1
    Possible duplicate of [How can I reverse an array in JavaScript without using libraries?](https://stackoverflow.com/questions/10168034/how-can-i-reverse-an-array-in-javascript-without-using-libraries) – Get Off My Lawn Jun 01 '18 at 16:20
  • Well. I didn't know about that.:D I just experiemented with shift(), pop(), unshift() and push(). But anyway; why is my code approach not working? I think I have got a general missunderstanding why this forEach-Loop isn't running correctly and thereforea working solution for this bug would fix other problems aswell. – Burg Jun 01 '18 at 16:23
  • My guess is that its not working because array.foreach() terminates too soon because you are modifying the length of oldArray while array.foreach() is iterating over it – spectacularbob Jun 01 '18 at 16:27

6 Answers6

2

Ok so Issue is your forEach which not running with oldArray length mean 5 times Why this is happening? It is because you are poping out value withing forEach which change the length and data of forEach.

You can check by putting console.log(oldArray); in forEach.

var arrayA = ["h", "e", "l", "l", "o"];
var arrayB = [];

function copyArray(oldArray, newArray) {
    var length = oldArray.length;
    for (var i = 0; i < length; i++) {
        var storeElement = oldArray.pop();
        newArray.push(storeElement);
    };
    console.log(oldArray + " old array");
    console.log(newArray + " new array");
}


copyArray(arrayA, arrayB);


var reverseArray = ["h", "e", "l", "l", "o"].reverse();

console.log("reverse array");

console.log(reverseArray);
Nishant Dixit
  • 5,388
  • 5
  • 17
  • 29
  • @DonOverflow Note how he stores `length` in a variable before the loop, this lets you change `oldArray` while looping through it. if you used `i < oldArray.length` as the second condition in that for loop you get the same propblem you had before – IrkenInvader Jun 01 '18 at 16:30
  • Thank you both! I marked this as the right answer. Didn't thought about the changing length. – Burg Jun 01 '18 at 16:35
0

When you are doing the pop, is over a new copy of your array, you are not modify the oldArray.

Jonathan Guerrero
  • 341
  • 1
  • 4
  • 8
0

You are changing the array while iterates it, because that you get this kind response.

To do this you can do in this way

var arrayA = ["h","e","l","l","o"];
var arrayB = [];
function copyArray(oldArray, newArray) {
  newArray = oldArray.reverse().slice();
  oldArray.length = 0;
  console.log(oldArray + " old array");
  console.log(newArray + " new array");
}


copyArray(arrayA, arrayB);
0

What you've encountered here is looping through an array while editing its contents.

For every element in the old array you pop an element and push it onto another element. Think about what happens when the forEach loop is on the nth element of the array and the n+1 th element is popped? It's now looping over an array that is one element shorter than before? Which element then becomes next in the forEach loop?

If you are wanting to remove all the elements in the first array use a while

while(oldArray.length !== 0){
    newArrary.push(oldArray.pop());
}

If you are wanting to preserve the original array loop over each element and add it to the new array

oldArray.forEach(function(oldArrayElement) {
    newArray.push(oldArrayElement);
});
Joel Anderson
  • 535
  • 8
  • 22
0

As pointed out you can use reverse. But to answer you question about why this doesn't work: You are modifying the array in place so the forEach loop exits early.

So one solution would to be:

  1. Don't modify the array you are looping through
  2. Get the last item from the oldArray and add it to the newArray
  3. Then the second-last item from oldArra and add it to the newArray

etc.

We know that we can get the last item from array with

var lastItem = myArray[myArray.length-1]

So to get the second last item we would do

var secondLastItem = myArray[myArray.length-2]

We can still use the forEach method, since the callback has few parameters like the curent item and the current index.

So when you loop through an array the first index is 0, next time the index becomes 1, etc.

Now we have something that increases as our loops continues and we can subtract 1 , then subtract 2, then subtract 3, etc.

So our solution could look like this;

var arrayA = ["h","e","l","l","o"];
var arrayB = [];
function copyArray(oldArray, newArray) {         
    oldArray.forEach(function(item,index) {                         
        newArray.push(oldArray[oldArray.length-1-index]); 
    });
    console.log(oldArray + " old array");
    console.log(newArray + " new array");
}

copyArray(arrayA, arrayB);
Bergur
  • 3,962
  • 12
  • 20
0

One way to use is push and another way is using spread operators,

oldArray = ["h", "e", "l", "l". "o"];
newArray = [];
newArray = [...oldArray.reverse()];

This is a simplest way of making a copy in reverse order.

Vijay
  • 228
  • 3
  • 8