Javascript: for-statement not processing all elements in array

Question

I have an array with various paragraphs inside:

var content = [ "<p>This is a paragraph 1</p>", 
                "<p>This is a paragraph 2</p>", 
                "<p>This is a paragraph 3</p>", 
                "<p>This is a paragraph 4</p>", 
                "<p>This is a paragraph 5</p>" ]

I want to perform a simple operation on each element inside the array. Each paragraph needs to be placed inside a container and then be removed from the array. This is the corresponding HTML:

<div class="article">
  <div id="cell1" class="text-block">
    <div class="container"></div>
  </div>
</div>

And this is how I am attempting to do what I describe above:

for (i = 0; i < content.length; i++) {
  console.log("Current item: " + content[i])
  //
  var itemtoRemove = content[i];
    $("#cell1 .container").append(content[i]);
  //
  content.splice($.inArray(itemtoRemove, content), 1)
}

I have put everything together in a fiddle as well: https://jsfiddle.net/johschmoll/1mjzu4qg/71/

Basically, everything is working fine, but the for-statement seems to be skipping every second element in the array. I cannot figure out the reason, even after looking at similar cases. What am I doing wrong here?

If you are going to manipulate the array while looping over it, you need to start at the `length - 1` index and move backwards. Because once you remove the first element and your `i` adjusts to 1, you're effectively looking at originally the 3rd element — Taplar, Jan 03 '20 at 16:42
`Array#splice` mutates the original array. Could it be that? — customcommander, Jan 03 '20 at 16:42
`content.splice` modifies the array while you're looping over it. — , Jan 03 '20 at 16:42
Otherwise, switch away from a for loop and use a `while (content.length)` loop — Taplar, Jan 03 '20 at 16:42
Why not use a [snippet](https://stackoverflow.blog/2014/09/16/introducing-runnable-javascript-css-and-html-code-snippets/)? — gman, Jan 03 '20 at 16:43
@Taplar Thanks, I was blind to that logic, it makes perfect sense — JoSch, Jan 03 '20 at 16:46
As a best practice, do not change the thing you are iterating on. Build a new array while iterating on the original one. It will certainly be easier to maintain/get what's happening instead of shifting indices. — sjahan, Jan 03 '20 at 16:46
@sjahan Thanks for the reply, can you explain why this is the best practice? — JoSch, Jan 03 '20 at 16:47
@JoSch Immutability is often a good thing: https://stackoverflow.com/questions/34385243/why-is-immutability-so-important-or-needed-in-javascript. If you want to change something, you create another thing and use it elsewhere. That way, you make sure that the data won't move without you are aware of it. It is often easier to understand, handle and maintain for your teammates later too! About iteration, some language would prevent you to modify the collection as it breaks the basis you are iterating, it is often a bad practice. You can edit an item, but not its order inside the collection. — sjahan, Jan 03 '20 at 16:50

Taplar · Accepted Answer · 2020-01-03T16:50:03.950

3

Since you are modifying the array as you loop, use a while loop that checks that the array is not empty

while (content.length) {
  console.log("Current item: " + content[0])
  //
  var itemtoRemove = content[0];
    $("#cell1 .container").append(content[0]);
  //
  content.shift();
}

Keeping in mind that if all you are doing is appending all the elements, then this is unnecessary. append() can accept an array

$("#cell1 .container").append(content.splice(0));

edited Jan 03 '20 at 16:50

answered Jan 03 '20 at 16:44

Taplar

24,788
4
22
35

Regarding your note: Well, I am only appending, but it is important that the appended elements get removed from the array – JoSch Jan 03 '20 at 16:48
So, append with the array and then set the array to an empty array. Two operations – Taplar Jan 03 '20 at 16:49
I should explain the context in which I'm using this: I'm working on a script that places paragraphs into a container until this container is "full", all paragraphs that do not fit get put into a separate container. That is why I need to modify the array. See here: https://jsfiddle.net/johschmoll/1mjzu4qg/96/ Happy about any and all advice on this. – JoSch Jan 03 '20 at 17:09
Ok, so in your example there, you are not simply appending the elements. You are conditionally appending the elements to different things. That's a different use case. The only thing I would say about that snippet is to point out the usage of `shift()` rather than `splice()` – Taplar Jan 03 '20 at 17:11
Yeah, thanks for pointing it out. `shift()` was new for me, always a pleasure learning something new. – JoSch Jan 03 '20 at 17:16

score 1 · Answer 2 · answered Jan 03 '20 at 16:45

Because you are modifying the array upon each iteration, the length of the array changes and you wind up skipping items. Instead, loop through the array backwards to avoid the issue.

var content = [ "<p>This is a paragraph 1</p>", 
                "<p>This is a paragraph 2</p>", 
                "<p>This is a paragraph 3</p>", 
                "<p>This is a paragraph 4</p>", 
                "<p>This is a paragraph 5</p>" ]

for (var i = content.length-1; i >= 0 ; i--) {
  console.log("Current item: " + content[i])

  var itemtoRemove = content[i];
  $("#cell1 .container").append(content[i]);

  content.splice($.inArray(itemtoRemove, content), 1)
}

<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.3.1/jquery.min.js"></script>
<div class="article">
  <div id="cell1" class="text-block">
    <div class="container"></div>
  </div>
</div>

Javascript: for-statement not processing all elements in array

2 Answers2