Editing <option> values through a textbox with jquery

Question

I've written a script that will allow me to enter a serial number for each received product, in-line in a table. First you enter the amount received, which increases the size of the <select> to that amount. Then you can enter new serial numbers through the s/n text box, submitting with TAB or Return.

Additionally, selecting one of the options (empty or filled) will allow you to edit that specific option - otherwise, the next empty line is taken when submitting a serial number. It also checks if the entered serial number already exists, which disallows entering it. This works with multiple products (each tbody is a product "row").

I'm looking for any tips to improve style, best practices and performance. Thanks in advance.

Here's a working JSFiddle (I've translated the comments, but classes etc. are still Dutch): http://jsfiddle.net/iv00w/7wds214d/

My HTML code:

<table id="BestelRegels">
    <tbody class="BestelRegel">
        <tr class="BestelRegelHead">
            <td class="AantalOntvangen">
                Amount: <input type="text" value="0" />
            </td>
        </tr>
        <tr class="BestelRegelSub">
            <td>omschrijving etc.</td>
        </tr>
        <tr class="BestelRegelSub">
            <th class="SerieNr">Enter s/n:</th>
        </tr>
        <tr class="BestelRegelSub">
            <td class="SerieNr">
                <input type="text" />
            </td>
        </tr>
        <tr class="BestelRegelSub">
            <td class="SerieNr">
                <select size="0"></select>
            </td>
        </tr>
    </tbody>
</table>

And my Javascript:

$(".AantalOntvangen input").each(function () {
    $(this).keyup(function (event) {
        var aantal = $(this).val();

        // find the select box that holds the s/n
        var serieNrBox = $(this).parent().parent().nextAll().last().find("select");

        // resize it
        serieNrBox.prop("size", aantal);

        // append new options until it matches the size
        while (serieNrBox.children().length < aantal) {
            serieNrBox.append($("<option></option>"));
        }

        // select the first option
        $(serieNrBox).children().first().prop("selected", true);
    });
});

$(".SerieNr input").each(function () {
    $(this).keydown(function (event) {
        if (event.which == 9 || event.which == 13) {
            var serieNrBox = $(this).parent().parent().next().find("select");
            var selected = $("option:selected", serieNrBox);
            var emptyOptions = serieNrBox.find("option")
                .filter(function () {
                return !this.value || $.trim(this.value).length === 0;
            });
            var serienr = $(this).val();
            var exists = false;

            // the s/n has to be unique
            serieNrBox.children().each(function () {
                if ($(this).val() == serienr) {
                    // error
                    exists = true;
                }
            });

            // if it is unique, update the selected, or next empty, option
            if (exists === false) {
                if (selected.val().length > 0) {
                    emptyOptions.first().prop("selected", true);
                } else {
                    emptyOptions.next().first().prop("selected", true);
                }

                selected.text(serienr);
                selected.val(serienr);

                $(this).val("").focus();
            }
        }
    });
});

$(".SerieNr select").each(function () {
    $(this).change(function () {
        // update the textbox
        var selected = $("option:selected", this);
        var serieNr = $(this).parent().parent().prev().find("input");
        serieNr.val(selected.val()).focus().select();
    });
});

Answer 1

First: Rename your variables etc. to English. This is probably why you have not received an answer for one of the most common topics in CR.

Bad Relative Traversal

This is bad practice:

var serieNrBox = $(this).parent().parent().nextAll().last().find("select");
//...
var serieNrBox = $(this).parent().parent().next().find("select");
//...
var serieNr = $(this).parent().parent().prev().find("input");

If you traverse the document tree like this you will need to do shotgun surgery each time anything changes in the DOM. For example, think what would have to change in the above code, if you (or the next developer who maintains this) has to add another select in the same cell as the current select.

Do not traverse DOM to like this to find an element. Give each element you need to find a class name and find relative to the container.

Better Organization

I understand you do this to support multiple product tbodys. You can instead do a:

$("#BestelRegels").find(".BestelRegel").each(function(index, orderRule) {
    //You can locate things relative to orderRule now...

which follows the logical organization of the document.

Unnecessary each

each function bodies with a single function call to $(this) is unnecessary:

$(".SerieNr select").each(function () {
    $(this).change(function () {

is equivalent to:

$(".SerieNr select").change(function () {

Applying these you get:

function alreadyExists(serieNrBox, serienr) {
    return serieNrBox.children().is(function () {
        return $(this).val() === serienr;
    });
}

$("#BestelRegels").find(".BestelRegel").each(function(index, orderRule) {

   // give these elements unique (within tbody) class names instead
   var serieNrBox = $("select", orderRule);
   var txtSerieNr = $(".SerieNr input", orderRule);
   var txtNumberReceived = $(".AantalOntvangen input", orderRule);

    txtNumberReceived.keyup(function (event) {
        var aantal = $(this).val();

        // resize it
        serieNrBox.prop("size", aantal);

        // append new options until it matches the size
        while (serieNrBox.children().length < aantal) {
            serieNrBox.append($("<option></option>"));
        }

        // select the first option
        serieNrBox.children().first().prop("selected", true);
    });


    txtSerieNr.keydown(function (event) {
        if (event.which !== 9 && event.which !== 13) return;

        var serienr = $(this).val();

        // the s/n has to be unique
        if (alreadyExists(serieNrBox, serienr)) return;

        // update the selected, or next empty, option
        var selected = $("option:selected", serieNrBox);

        selected.text(serienr);
        selected.val(serienr);

        var emptyOptions = serieNrBox.find("option")
            .filter(function () {
            return !this.value || $.trim(this.value).length === 0;
        });

        (selected.val().length > 0 
            ? emptyOptions 
            : emptyOptions.next())
                .first()
                .prop("selected", true);

        $(this).val("").focus();
    });

    serieNrBox.change(function () {
        // update the textbox
        var selected = $("option:selected", this);
        txtSerieNr.val(selected.val()).focus().select();
    });
});

Notes:

• References to parent and children are gone (except for serieNrBox.children() which is quite safe).

• keydown handler is simplified.

• You can check if any element in a selection satisfies some condition using is.