folks! Today I created this script that has the following functionality:

• add new items to array
• list all items from the array
• remove an item from the array

There are two functions:

• addToFood() - adds the value of input to the array and updates innerHTML of div
• removeRecord(i) - remove a record from the array and updates innerHTML of div

The code includes 3 for loops and you can see it at - /

My Master told me that those 3 for loops make the solution way to heavy. Is there a better way to do the same thing? Is it better to decrease the loops and try to use splice? Thanks in advance.

HTML

<!-- we add to our foodList from the value of the following input -->    
<input type="text" value="food" id="addFood" />

<!-- we call addToFood(); through the following button  -->    
<input type="submit" value="Add more to food" onClick="addToFood();">

<!-- The list of food is displayed in the following div -->    
<div id="foods"></div>

JavaScript

var foodList = [];

function addToFood () {
    var addFood = document.getElementById('addFood').value;
    foodList.push(addFood);

    for (i = 0; i < foodList.length; i++)   {
        var newFood = "<a href='#' onClick='removeRecord(" + i + ");'>X</a> " + foodList[i] + " <br>";
    };
    document.getElementById('foods').innerHTML += newFood;
}


function removeRecord (i) {

    // define variable j with equal to the number we got from removeRecord
    var j = i;

    // define and create a new temporary array
    var tempList = [];

    // empty newFood
    // at the end of the function we "refill" it with the new content
    var newFood = "";
    for (var i = 0; i < foodList.length; i++) {
        if(i != j) {

    // we add all records except the one == to j to the new array
    // the record eual to j is the one we've clicked on X to remove
            tempList.push(foodList[i]);
        } 
    };

    // make redefine foodList by making it equal to the tempList array
    // it should be smaller with one record
    foodList = tempList;

    // re-display the records from foodList the same way we did it in addToFood()
    for (var i = 0; i < foodList.length; i++) {
        newFood += "<a href='#' onClick='removeRecord(" + i + ");'>X</a> " + foodList[i] + " <br>";
    };
    document.getElementById('foods').innerHTML = newFood;
}

folks! Today I created this script that has the following functionality:

• add new items to array
• list all items from the array
• remove an item from the array

There are two functions:

• addToFood() - adds the value of input to the array and updates innerHTML of div
• removeRecord(i) - remove a record from the array and updates innerHTML of div

The code includes 3 for loops and you can see it at - http://jsfiddle.net/menian/3b4qp/1/

My Master told me that those 3 for loops make the solution way to heavy. Is there a better way to do the same thing? Is it better to decrease the loops and try to use splice? Thanks in advance.

HTML

<!-- we add to our foodList from the value of the following input -->    
<input type="text" value="food" id="addFood" />

<!-- we call addToFood(); through the following button  -->    
<input type="submit" value="Add more to food" onClick="addToFood();">

<!-- The list of food is displayed in the following div -->    
<div id="foods"></div>

JavaScript

var foodList = [];

function addToFood () {
    var addFood = document.getElementById('addFood').value;
    foodList.push(addFood);

    for (i = 0; i < foodList.length; i++)   {
        var newFood = "<a href='#' onClick='removeRecord(" + i + ");'>X</a> " + foodList[i] + " <br>";
    };
    document.getElementById('foods').innerHTML += newFood;
}


function removeRecord (i) {

    // define variable j with equal to the number we got from removeRecord
    var j = i;

    // define and create a new temporary array
    var tempList = [];

    // empty newFood
    // at the end of the function we "refill" it with the new content
    var newFood = "";
    for (var i = 0; i < foodList.length; i++) {
        if(i != j) {

    // we add all records except the one == to j to the new array
    // the record eual to j is the one we've clicked on X to remove
            tempList.push(foodList[i]);
        } 
    };

    // make redefine foodList by making it equal to the tempList array
    // it should be smaller with one record
    foodList = tempList;

    // re-display the records from foodList the same way we did it in addToFood()
    for (var i = 0; i < foodList.length; i++) {
        newFood += "<a href='#' onClick='removeRecord(" + i + ");'>X</a> " + foodList[i] + " <br>";
    };
    document.getElementById('foods').innerHTML = newFood;
}

• javascript
• arrays

4 Answers 4

You should use array.splice(position,nbItems)

function removeRecord (i) {
    foodList.splice(i, 1); // remove element at position i
    var newFood = "";
    for (var i = 0; i < foodList.length; i++) {
        newFood += "<a href='#' onClick='removeRecord(" + i + ");'>X</a> "
            + foodList[i] + " <br>";
    };
    document.getElementById('foods').innerHTML = newFood;
}

http://jsfiddle.net/3b4qp/5/

Now using JQuery:

$(function(){
    $(document).on('click','input[type=submit]',function(){
        $('#foods')
           .append('<div><a href="#" class="item">X</a> ' 
                + $('#addFood').val() + '</div>');
    });

    $(document).on('click','.item',function(){
        $(this).parent().remove();
    });
});

http://jsfiddle.net/jfWa3/

Your problem isn't the arrays, your problem is this code:

node.innerHTML += newFood;

This code is very, very, very slow. It will traverse all exising DOM nodes, create strings from them, join those strings into one long string, append a new string, parse the result to a new tree of DOM nodes.

I suggest to use a framework like jQuery which has methods to append HTML fragments to existing DOM nodes:

var parent = $('#foods');
...
for (var i = 0; i < foodList.length; i++) {
    parent.append( "<a href='#' onClick='removeReco..." );

That will parse the HTML fragments only once.

If you really must do it manually, then collect all the HTML in a local string variable (as suggested by JohnJohnGa in his answer) and then assign innerHTML once.

Here's some tips to, at least, make your code more portable (dunno if it will be better performance wise, but should be, since DOM Manipulation is less expensive)

Tips

1. First separate your event handle from the HTML
2. Pass the "new food" as a function paramater
3. Tie the array elements to the DOM using the ID
4. Instead of rerendering everything when something changes (using innerHTML in the list), just change the relevant bit

Benefits:

1. You actually only loop once (when removing elements from the array).
2. You don't re-render the list everytime something changes, just the element clicked
3. Added bonus: It's more portable.
4. Should be faster

Example code:

FIDDLE

HTML

<div id="eventBinder">
    <!-- we add to our foodList from the value of the following input -->
    <input id="addFood" type="text" value="food" />
    <!-- we call addToFood(); through the following button -->
    <button id="addFoodBtn" value="Add more to food">Add Food</button>
    <!-- The list of food is displayed in the following div
    -->
    <div id="foods"></div>
</div>

JS

// FoodList Class
var FoodList = function (selectorID) {
    return {
        foodArray: [],
        listEl: document.getElementById(selectorID),
        idCnt: 0,
        add: function (newFood) {
            var id = 'myfood-' + this.idCnt;
            this.foodArray.push({
                id: id,
                food: newFood
            });
            var foodDom = document.createElement('div'),
                foodText = document.createTextNode(newFood);
            foodDom.setAttribute('id', id);
            foodDom.setAttribute('class', 'aFood');
            foodDom.appendChild(foodText);
            this.listEl.appendChild(foodDom);
            ++this.idCnt;
        },
        remove: function (foodID) {
            for (var f in this.foodArray) {
                if (this.foodArray[f].id === foodID) {
                    delete this.foodArray[f];
                    var delFood = document.getElementById(foodID);
                    this.listEl.removeChild(delFood);
                }
            }
        }
    };
};

//Actual app
window.myFoodList = new FoodList('foods');

document.getElementById('eventBinder').addEventListener('click', function (e) {
    if (e.target.id === 'addFoodBtn') {
        var food = document.getElementById('addFood').value;
        window.myFoodList.add(food);
    } else if (e.target.className === 'aFood') {
        window.myFoodList.remove(e.target.id);
    }
}, false);

Here is another sugestion:

function remove(arr, index) {
  if (index >= arr.lenght) { return undefined; }
  if (index == 0) {
    arr.shift();
    return arr;
  }

  if (index == arr.length - 1) {
    arr.pop();
    return arr;
  }
  var newarray = arr.splice(0, index);
  return newarray.concat(arr.splice(1,arr.length))
}