Remove item from Javascript array and display new array

Question

This simple code allows me to display a list of items in an array, and to delete a chosen item from the array, and finally display a new list without the deleted item.

var carBrands = ["Toyota", "Honda", "BMW", "Lexus", "Mercedes","Peugeot","Aston   Martin","Rolls Royce"];
var html="";
var newList="";
var itemToRemove;

$(document).ready (function () {
console.log("Ready to go!");
$("#displayList").bind('click', function(event) {
    displayList();
   });
$("#removeItem").bind('click', function(event) {
    item = document.getElementById("input").value;
    removeItemFromList(item);
});
 });
function displayList() {
for (var i = 0; i < carBrands.length; i++) {
    html+= "<li>" + carBrands[i] + "</li>";
    document.getElementById("list").innerHTML=html;
}

}
function removeItemFromList(item) {
itemToRemove = item;

for (var i=0; i<carBrands.length; i++) {
    if (itemToRemove == carBrands[i]) {
        carBrands.splice(carBrands[i], 1);
    }
    newList+= "<li>" + carBrands[i] + "</li>";
    document.getElementById("newList").innerHTML=newList;
}
}

This works fine on the first try,

enter image description here

but if i try again the new list is appended to the old list and the item i removed initially is also added to it.

enter image description here

My Question:

How do i display the list without the removed items?
Are html and newList objects?

Paul Roub · Accepted Answer · 2014-08-09 20:48:59Z

2

First, splice() doesn't want the thing you're removing, it wants the index of the thing.

So, instead of:

carBrands.splice(carBrands[i], 1);

you want:

carBrands.splice(i, 1);

And you're never emptying newList after deleting the last item, so you're just re-adding everything to the list.

Try this instead:

function removeItemFromList(item) {
  itemToRemove = item;

  for (var i=0; i < carBrands.length; i++) {
    if (item == carBrands[i]) {
      carBrands.splice(i, 1);
      break;
    }
  }

  newList = "<li>" + carBrands.join('</li><li>') + "</li>";  
  document.getElementById("newList").innerHTML = newList;
}

Also, you'll notice that itemToRemove was never needed in the loop; if you're not using it elsewhere, it can just go away.

answered Aug 9, 2014 at 20:48

Paul Roub

36.5k27 gold badges88 silver badges95 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

Paul Roub Over a year ago

And no, html and newList are strings, not objects.

some Over a year ago

What's wrong with carBrands.indesOf(item) ? And why create the list in both removeItemFromList and displayList?

Kyle Falconer · Accepted Answer · 2014-08-09 21:00:25Z

1

Your problem has to do with variable scope: the html and newList variables retained their values each time the functions were run (aka: each time the buttons were pressed).

You also had a severe performance issue. When looping, you don't want to update the DOM on each iteration, and you want to update the DOM as few times as is necessary.

Don't do:

var html = "";
function displayList() {
    html = "";
    for (var i = 0; i < carBrands.length; i++) {
        html += "<li>" + carBrands[i] + "</li>";
        document.getElementById("list").innerHTML = html;
    }    
}

Do:

function displayList() {
    var html = "";
    for (var i = 0; i < carBrands.length; i++) {
        html += "<li>" + carBrands[i] + "</li>";
    }
    document.getElementById("list").innerHTML = html;
}

Here is my jsfiddle of the solution: http://jsfiddle.net/netinept/jmm93fy1/

And the complete resulting JavaScript (including Paul Roub's join solution for constructing the final list):

var carBrands = ["Toyota", "Honda", "BMW", "Lexus", "Mercedes", "Peugeot", "Aston Martin", "Rolls Royce"];


var itemToRemove;

$(document).ready(function () {
    console.log("Ready to go!");
    $("#displayList").bind('click', function (event) {
        displayList();
    });
    $("#removeItem").bind('click', function (event) {
        item = document.getElementById("input").value;
        removeItemFromList(item);
    });
});

function displayList() {
    var html = "";
    for (var i = 0; i < carBrands.length; i++) {
        html += "<li>" + carBrands[i] + "</li>";
    }
    document.getElementById("list").innerHTML = html;
}

function removeItemFromList(item) {
    itemToRemove = item;
    for (var i = 0; i < carBrands.length; i++) {
        if (itemToRemove == carBrands[i]) {
            carBrands.splice(i, 1);
            break;
        }
    }
    var newList = "<li>" + carBrands.join('</li><li>') + "</li>";
    document.getElementById("newList").innerHTML = newList;
}

edited Aug 9, 2014 at 21:00

answered Aug 9, 2014 at 20:50

Kyle Falconer

8,5706 gold badges53 silver badges69 bronze badges

5 Comments

Paul Roub Over a year ago

You'll run into trouble if the last item in the list is removed; the carBrands[i] reference in the next line will be undefined.

Kyle Falconer Over a year ago

@PaulRoub You're right, I overlooked that and fixed it. I had thought of doing the join method, but for retaining much of the original code, I went with this other solution.

Paul Roub Over a year ago

The problem with your current fix is this: say item 0 is removed. Now there's a new item 0, but next time through the loop we're adding item 1 and leaving 0 out of the list. This stuff is what led me to go with join() :-)

Kyle Falconer Over a year ago

@PaulRoub oh balls. You're right again. I don't work with splice very often, so I overlooked that it messes with the index. I "borrowed" your solution :P

some Over a year ago

Whats wrong with indexOf? And why concatenating string when you can use the dom? jsfiddle.net/some/jfdm2yry

Collectives™ on Stack Overflow

Remove item from Javascript array and display new array

2 Answers 2

2 Comments

Don't do:

Do:

5 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

2 Comments

Don't do:

Do:

5 Comments

Your Answer

Sign up or log in

Post as a guest

Related