8
\$\begingroup\$

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

/// 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.prop("type") !== "password") {
                val = $.trim(val);
            }

            // Focus (select) first empty field if any.
            if (val === "" && !emptyInputs++) {
                $this.select();
            }
        });

        if (emptyInputs === 0) {
            var $button = $("#signup button");

            $button.prop("disabled", true);

            return true;
        }

       return false;
    });
});
\$\endgroup\$
3
\$\begingroup\$

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);
\$\endgroup\$
  • \$\begingroup\$ Thank you, once again! The end result is great (see my answer). It really looks like clean code now. A note: $div.has(".error").length > 0 in your answer should be $div.has(".error").length === 0. I've also declared the variable $form = $("#signup"); on top, and I'm using find() to get every element I need from it. So if the form ID changes I only have to change it at one place. Lastly, I'm unfamiliar with JS file names, but I was thinking about naming it something like signup-handler.js, so for my login it would be login-handler.js. Do you think that would be appropriate? \$\endgroup\$ – Kid Diamond Sep 6 '14 at 11:34
  • \$\begingroup\$ ^ You can improve it further; $div.has(".error").length === 0 can simply be !$div.has(".error").length. \$\endgroup\$ – Ben Sep 6 '14 at 12:52
  • \$\begingroup\$ @Ben I'd be hesitant to call that an improvement; it may be shorter but it isn't necessarily more readable. I'm biased toward my way (naturally :P) because I can read it out more easily as simply "div has error length greater than zero." However, @ KidDiamond, if you choose to implement that, you can do so with both checks in the first snippet as well. \$\endgroup\$ – Schism Sep 6 '14 at 16:05
  • 1
    \$\begingroup\$ If it's greater than zero, then it's true; if not it's false. Seems easier to me to test against the length property being truthy or not. You could read it out as "div has error length". :-) \$\endgroup\$ – Ben Sep 6 '14 at 17:51
1
\$\begingroup\$

Having followed your progress from the code of your last question, I applaud you. Your current version looks much better! However I might give you some additional suggestions.

  • You might want to start linting your JavaScript code. It formalizes your code style and can reveal additional possible optimizations.

  • I wonder why you test against maxCharLengthFirstName when you already have maxlength set on the first name input element. This seems redundant. If you need to keep it somehow you could get it from the attribute instead of depending on a magic number, e.g.: var maxCharLengthFirstName = $firstName.prop("maxlength")

  • You could pass a selector to .on(). You could write $form.on("focus blur", "input", function(e) {...}) instead of $form.find("input").on("focus blur", function(e) {...}) which should be marginally faster as it uses event bubbling. Note: You’d probably need to change you inner code to refer to e.currentTarget instead of this.

  • You could unify your selectors: You’re using classes, attributes and tag selectors. You might benefit from using a common convention in your HTML that describes its relation to the jQuery code.

  • Your JavaScript code is still tightly coupled to a certain HTML structure and you are doing a lot of DOM traversal (.find(), .siblings(), …). Chances are, if your HTML changes, that your JavaScript will break. You might want to re-evaluate your hard-coded DOM references.

  • You are using .hide() and .css(), which create inline styles. This could be handled solely by CSS. From a separation of concerns perspective this would be cleaner. Simply add/remove class names instead.

  • You could convert your code into a jQuery plugin that allows for re-use. jQuery Boilerplate would be a good starting point.

  • You might want to wrap your code in a self executing anonymous function. This is a common pattern to ensure correct variable scope. (function(window, document, $, undefined) {...})(window, document, jQuery);

\$\endgroup\$
  • \$\begingroup\$ What do you mean by "unifying" my selectors? Isn't jQuery supposed to be tightly coupled with HTML? It's obvious to me that when the HTML changes, the JS code breaks. I'm okay with using hide() and .css(). Using classes instead is great for Separation of Concerns, but not for readability in the JS code, imo. \$\endgroup\$ – Kid Diamond Sep 6 '14 at 18:52
  • \$\begingroup\$ Unifying selectors would be micro-optimization, I meant establishing a convention in your HTML that communicates more clearly that you are altering elements with js, something like .js-form-input-first-name for instance. \$\endgroup\$ – mlnmln Sep 6 '14 at 19:23
  • \$\begingroup\$ Ah, I see. As far as the self executing anonymous function, I don't think I would gain any benefit from that. But you have some great other points that I didn't know about yet. \$\endgroup\$ – Kid Diamond Sep 6 '14 at 19:26
  • 1
    \$\begingroup\$ The self executing anonymous function prevents you from having to use jQuery in noConflict mode if another framework also uses $ as their shortcut variable name. api.jquery.com/jQuery.noConflict \$\endgroup\$ – mlnmln Sep 6 '14 at 19:42
  • 1
    \$\begingroup\$ Got me with this one! The goto solution would be using CSS3 transitions. However mimicking the behavior of .slideDown() and .slideUp() using CSS only is surprisingly difficult as you would need to animate an unknown height. There are a couple of less than optimal solutions out there, so for now I'd leave it as is. \$\endgroup\$ – mlnmln Sep 7 '14 at 9:58
1
\$\begingroup\$

Having applied the feedback I've gotten on this and my first question, this is the end result:

signup-form-handler.js

(function ($) {
    "use strict";

    $(function () {
        var $form      = $("#signup"),
            $errors    = $form.find(".error"),
            $firstName = $form.find("input[name=firstName]"),
            $lastName  = $form.find("input[name=lastName]");

        // Hide all input messages except errors.
        $form.find(".addition:not(.error)").parent().hide();

        // Auto select first eligible input.
        if (!$errors.length) {
            $firstName.select().siblings("div").show();
        } else if ($errors.first().closest(".input").has($firstName).length) {
            if (!$firstName.val() || $firstName.val().length > $firstName.prop("maxlength")) {
                $firstName.select();
            } else {
                $lastName.select();
            }
        } else {
            $errors.first().closest(".input").children("input").select();
        }

        // When input is focussed / blurred,
        // toggle slide on input message except error.
        $form.on("focus blur", "input", function (e) {
            var $div = $(this).siblings("div");

            if (!$div.has(".error").length) {
                e.type === "focusin" ? $div.stop().slideDown("fast") : $div.stop().slideUp("fast");
            }
        });

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

            // Select first empty input if any.
            $form.find("input").each(function () {
                var $this = $(this),
                    value = $this.val();

                if ($this.prop("type") !== "password") {
                    value = $.trim(value);
                }

                if (value === "" && !emptyInputs++) {
                    $this.select();
                }
            });

            if (emptyInputs) {
                // Prevent form submission.
                return false;
            }

            // Disable submit button to prevent multiple submissions.
            $form.find("button").prop("disabled", true);
        });
    });
})(jQuery);
\$\endgroup\$
  • 1
    \$\begingroup\$ signup-handler.js and form-handler.js both sound good to me! Nice job :) \$\endgroup\$ – Schism Sep 6 '14 at 16:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.