Sanitizing and validating a user signup form before submission

The first part of the code (above the in-code comment in the middle) looks messy.

I know I could use variables to make it cleaner, I just don't exactly know yet how can I effectively apply those when dealing with jQuery objects. So I'd appreciate a review on that.

As for the if-statements, I think they're okay but if they could be improved I'm all open for it.

Any other suggestions are always welcome.

```
$(function() {
// magic number
var MAX_FIRST_NAME_LENGTH = 35;

// Hide additional input messages by default except errors.
$("#signup .addition:not(.error)").css("display", "inline-block").parent().hide();

// Select (focus) first input and show it's additional (erroneous) message.
if ($("#signup .error").length === 0) {
$("#signup input[name=firstName]").select();
$("#signup input[name=firstName]").siblings("#signup div").show();
} else if ($("#signup .error").first().parent("#signup div").siblings($("#signup input[name=firstName]")).val().length >= 1) {
if ($("#signup input[name=firstName]").val().length > MAX_FIRST_NAME_LENGTH) {
$("#signup input[name=firstName]").select();
} else {
$("#signup input[name=lastName]").select();
}
} else {
$("#signup .error").first().parent("#signup div").siblings($("#signup input")).select();
}

// Toggle slide on the additional message focus / blur of corresponding input.
$("#signup input").on("focus blur", function(e) {
if (!$(this).siblings("#signup div").children("#signup .addition").hasClass("error")) {
$(this).siblings("#signup div").stop()e.type === "focus" ? "slideDown" : "slideUp";
}
});

/// I think the code below is good as is.

var $form = $("#signup");

$form.on("submit", function() {
var emptyInputs = 0;

$form.find("input").each(function() {
var $this = $(this),
val = $this.val();

if ($this

Kid Diamond! It's always great to see code improving over time. ☺

On loading, you treat input[name=firstName] and input[name=lastName] specially, due to the need for some specialised logic. Instead of selecting each of them every time, you can assign them to variables. The variables are prefixed with $ to remind you that these are jQuery objects.

var $firstName = $("#signup input[name=firstName]"),
    $lastName  = $("#signup input[name=lastName]");

In the first if block, you could optionally take advantage of method chaining. I get rid of the #signup in the selector: since $firstName is already a descendant of #signup, then its siblings will be too, and so it's unnecessary.

$firstName.select().siblings("div").show();

The else if condition is not immediately obvious. After looking at it more carefully, the logic flow is this:

• Consider whether the error is associated with $firstName.

• If so, consider whether $firstName is not empty.

• If $firstName is not empty, then consider whether it is greater than the max length.

• If so, then $firstName is the errant field and should be selected.

• Otherwise, $firstName is fine, so $lastName must be errant and selected.

• If $firstName is empty, then fall-back to the else (below).

• Else, select the input associated with the first error.

This is actually rather complicated. In fact, for this snippet, I prefer your original code. Here is how I would rewrite it:

var $errors = $("#signup .error");
if ($errors.length === 0) {
    $firstName.select().siblings("div").show();
} else {
    var $firstError = $errors.first();
    if ($firstError.closest(".input").has($firstName).length > 0) {
        if (!$firstName.val() || $firstName.val().length > MAX_FIRST_NAME_LENGTH) {
            $firstName.select();
        } else {
            $lastName.select();
        }
    } else {
        $firstError.closest(".input").children("input").select();
    }
}

In the event handler, you can at least do var $div = $("#signup div"). Personally, I'd rather rewrite like the following, just because the logic reads slightly more clearly to me:

$("#signup input").on("focus blur", function(e) {
    var $div = $(this).siblings("#signup div");
    if ($div.has(".error").length > 0) {
        $div.stop();
        if (e.type === "focus") {
            $div.slideDown("fast");
        } else {
            $div.slideUp("fast");
        }
    }
});

Below the comment, the code is pretty good. The one thing I'd change is I wouldn't make a $button variable at all, since it's only used once. I might also reverse the order of the logic to avoid the unnecessary return true;:

if (emptyInputs > 0) {
    return false;
}
// if the form submits successfully, disable the button to prevent accidental double-submits.
$("#signup button").prop("disabled", true);

StackExchange Code Review Q#62042, answer score: 3