Making an array editable by user

What I want to do:

-
Let users add data to an array through an input field and add button.

HTML:

Add Number

JS:

var arrayList = [];     

$(document).ready(function() {

    $('#btnAdd').on('click', mkArray);

    $('#outputBox').on('click', 'span', deleteNum);

});

-
Display the array to the user.

function mkArray(event) {

    event.preventDefault();

    var numberAdd = $('#input').val();

    arrayList.push(numberAdd);

    $('#outputBox').append(' ' + numberAdd + '');

}

-
If the user clicks on the data, confirm deletion and then delete the item and refresh the array and display.

function deleteNum() {

    var confirmation = confirm('Are you sure you want to delete?');

    var numberIndex = $(this).index();

        if (numberIndex > -1) {

        arrayList.splice(numberIndex, 1);

        };

    $(this).remove();

};

JS FIDDLE

Question

• Any potential pitfalls with this way of modifying an array?

Any potential pitfalls with this way of modifying an array?

There certainly are for the user! Your code is not really confirming deletion: The number gets removed even if you press No/Cancel. You need to actually check the return value of the confirm() call:

var confirmation = confirm('Are you sure you want to delete?'); // returns true/false

if(confirmation) {
  // user confirmed, so proceed with removing the item
}

I'd also recommend a more descriptive confirmation message; tell me what I'm about to delete at least. Otherwise, I can't really be sure.

And there are further pitfalls.

For one, you're using the user's input as an element ID. Now, firstly, you don't need an ID. It's not used for anything. Second, IDs are really meant to be unique, but I can add the same thing as many times as I want, and I'll just get a bunch of elements with the same ID (it won't break anything, because - as mentioned - the ID isn't used for anything, but it only makes the IDs more pointless). And third, the input isn't being checked, so the user can input whatever they want, and have it appended directly to the page.

For instance, if you input alert("hacked"), you'll get an alert when you click to add it, because you've just added a script element to the page. It'll also add some complete nonsense HTML to the page, because a script tag has suddenly become the ID for a span element, and... well, nonsense. Even if you weren't using the user's input as ID, it'd still behave as described, because you're inserting the input string as-is in the span. If you want to insert as text, use jQuery's .text() to set it. Otherwise it'll be interpreted as HTML.

E.g. you can input the string $("body").empty() and things get very zen.

You can do all this with the JS console of course, but the point remains: Always, always validate and sanitize user input, and insert it in a safe manner.

Also, your arrayList variable is global, meaning anything and everything can modify it in any way (for instance, you can input arrayList = []; and your list will get reset when it should just add something). It can end up not even being an array. In that case, your code will fail, because you can't splice() something that's not an array. Even if it is still an array, it may have been sorted, emptied, or who knows what else, and splice() might remove the wrong thing - or nothing at all.

Really, the trouble here is that the array isn't doing anything useful. You add numbers to the HTML directly, and you remove them from the HTML directly. The array is just a weak, error-prone copy of what's on the page. And you have to manually keep them in sync.

It's much better to have a single source for your data, so there's only one place to keep track of things. In this case, I'd say that that source should be the HTML. If you need to get an array of inputs, you can just use jQuery to fetch all the input so far:

$("#outputBox span").map(function () { return $(this).text() }).toArray()

That line will go through each of the spans, get their text, and return an array with the stuff that's been added so far (provided you remove the extra space in the span, which shouldn't really be there anyway - see below)

Other stuff:

• arrayList is not a great name for a variable. Yeah, it's an array, so it's list of some sort... but what's in it?. I'd simply call it numbers or inputs, since that's what it actually represents.

• Use jQuery to build the elements to avoid the error-prone string-concatenation

• If you're making a list in HTML, make an actual ` or list (I'd pick ul as it's an unordered list of user input), and make the individual numbers list items (
• ).






• Doesn't element. A element is - by definition - already a button.






• You don't need event.preventDefault(), since clicking a button has no default behavior that needs preventing.






• Use the $(function () { ... }); shorthand instead of $(document).ready(function () { ... });`

In the end, you can do it all like this

HTML

Add

JS

$(function () {
  // keep the input and output elements around for later
  var input  = $("#input");
  var output = $("#output").on("click", "li", function () {
    if( confirm("Are you sure you want to delete this item?") ) {
      $(this).remove();
    }
  });

  $("#addItem").on("click", function () {
    var value = input.val();
    if(/^\d+$/.test(value)) { // check that the string contains only digits
      output.append($("").text(value));
      input.val("");
    } else {
      alert("Please enter only numbers");
    }
  });
});

Here's a demo

StackExchange Code Review Q#51958, answer score: 4