Specifics of for loop and remove()

Question

I have multiple html elements, which contain a number.
Each click on button reduces that number by -1.
When the number equals 0, element must be removed.

My code removes only the half of elements, and I don't understand why...
JsFiddle DEMO

<div class='moo'>3</div>
<div class='moo'>3</div>
<div class='moo'>3</div>
<div class='moo'>3</div>
<div class='moo'>3</div>
<div class='moo'>3</div>
<div class='moo'>3</div>
<div class='moo'>3</div>
<div class='moo'>50</div>

<button id='bubu'>Remove</button>

JS:

$(document).on('click','#bubu',function(){
  var x = document.getElementsByClassName('moo');
  for(var i = 0; i < x.length; i++){
    if( Number(x[i].innerHTML) > 0 ) {
      x[i].innerHTML = Number(x[i].innerHTML) - 1;
    }
    if( Number(x[i].innerHTML) == 0 ){
      x[i].remove();
    } 
  }
});

(i) Removing with replace and innerHTML is impossible, this is a simplified html.

Your data depends on DOM, which is very bad practice, You should use DOM to show things, not as a storage. Imagine if you could change your balance by inspecting element... — Ertan Kara, Mar 16 '19 at 13:51

Scott Marcus · Accepted Answer · 2019-03-16T14:02:54.857

It's because as you remove items, the length of the x node list gets smaller and you are using that length as the condition to loop until.

If you remove items from the end of the node list and work backwards, this approach will work because removing from the end allows the loop condition to shrink without skipping over any elements in the node list:

$(document).on('click','#bubu',function(){
  var x = document.getElementsByClassName('moo');
  for(var i = x.length-1; i >= 0 ; i--){
    if( Number(x[i].innerHTML) > 0 ) {
      x[i].innerHTML = Number(x[i].innerHTML) - 1;
    }
    if( Number(x[i].innerHTML) == 0 ){
      x[i].remove();
    } 
  }
});

<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.3.1/jquery.min.js"></script>
<div class='moo'>3</div>
<div class='moo'>3</div>
<div class='moo'>3</div>
<div class='moo'>3</div>
<div class='moo'>3</div>
<div class='moo'>3</div>
<div class='moo'>50</div>
<div class='moo'>3</div>
<div class='moo'>3</div>
<button id='bubu'>Remove</button>

Having said that, there are a number of problems with your approach:

.getElementsByClassName() returns a "live" node list and really shouldn't be used. Read another post of mine which explains this in more detail.

.innerHTML has security and performance implications and should only be used when the string you are working with contains HTML. Use .textContent when there is no HTML in the string.

Instead of relying on numeric counters that you have to manage with loops, use Arrays and its built-in .forEach() method for iteration.

Here's the same result, done in a more concise and performant way:

$('#bubu').on('click', function(){
  // Get the elements into an Arrray without a live node list
  var elements = Array.prototype.slice.call(document.querySelectorAll(".moo"));
  
  // Loop over the array
  elements.forEach(function(element){
    // Get the text of the element and convert to a number. The prepended + symbol does this.
    let elNum = +element.textContent;
    
    // Instead of two consecutive if/then statements, use one with an else if
    if(elNum > 0) {
      element.textContent = --elNum;  // Just set the content to 1 less
    } else if(elNum === 0){
      element.remove();
    }
  });
});

.moo { display:inline-block; font-size:1.5em; color:#f00; border:1px solid grey; }

<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.3.1/jquery.min.js"></script>
<div class='moo'>3</div>
<div class='moo'>3</div>
<div class='moo'>4</div>
<div class='moo'>3</div>
<div class='moo'>3</div>
<div class='moo'>3</div>
<div class='moo'>50</div>
<div class='moo'>3</div>
<div class='moo'>5</div>
<button id='bubu'>Remove</button>

score 1 · Answer 2 · answered Mar 16 '19 at 13:59

getElementsByClassName is returning a dynamic collection, so as you remove items from the DOM the size of the collection is reducing, the simplest way to solve this is to covert the collection into a standard array

var x = [].slice.call(document.getElementsByClassName('moo'))

Specifics of for loop and remove()

2 Answers2