HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Editing <option> values through a textbox with jquery

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
javascriptjquerycodereviewstackoverflow
editingwithoptiontextboxjquerythroughvalues

Problem

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 ` 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:


    
        
            
                Amount: 
            
        
        
            omschrijving etc.
        
        
            Enter s/n:


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 "));
}

// 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

Solution

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 "));
        }

        // 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.

Code Snippets

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");
$("#BestelRegels").find(".BestelRegel").each(function(index, orderRule) {
    //You can locate things relative to orderRule now...
$(".SerieNr select").each(function () {
    $(this).change(function () {
$(".SerieNr select").change(function () {
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();
    });
});

Context

StackExchange Code Review Q#62509, answer score: 2

Revisions (0)

No revisions yet.