5
\$\begingroup\$

The following JS code handles when the form is submitted and some little extra stuff.

I'd like to have it reviewed on the following aspects:

  • Readability
  • Efficiency
  • Usability
  • OOP

It works exactly as intended. I have commented the code to possibly clarify some things and also added the form's HTML code, just in case. It might get you to understand the code easier.

Any other suggestions are always welcome.

Note: I'm still a beginner with JavaScript / jQuery. A lot of things are still not obvious to me. Take that into consideration when assessing my code please.

jQuery

$(function() {
    // On inital document load.
    // Hide additional input messages by default except errors.
    $("#signup .addition").each(function() {
        if (!$(this).hasClass("error")) {
            $(this).parent("#signup div").hide();
            $(this).css("display", "inline-block");
        }
    });

    // If there are no error messages,
    // just select first input and show it's additional message.
    if ($("#signup .error").length === 0) {
        $("#signup input[name=firstName]").select();

        $("#signup input[name=firstName]").siblings("#signup div").show();
    } else {
        // There are error messages.
        // Focus (select) the first error input field.

        // If statement for these two inputs because they share the same container.
        if ($("#signup .error:first").parent("#signup div").siblings("input[name=firstName]").length >= 1) {
            // Check one against the validation rules to know which of the two to focus (select).
            if ($("#signup input[name=firstName]").val() && $("#signup input[name=firstName]").val().length <= 35) {
                $("#signup input[name=lastName]").select();
            } else {
                $("#signup input[name=firstName]").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");
        }
    });

    $("#signup").on("submit", function() {
        // Select first empty input if any, and don't submit.
        $("#signup input").each(function() {
            if ($(this).attr("type") !== "password") {
                if (!$.trim($(this).val())) {
                    $(this).select();

                    return false;
                }
            } else {
                // Password can contain spaces so no usage of trim for that.
                if (!$(this).val()) {
                    $(this).select();

                    return false;
                }
            }
        });

        // Calculate empty fields.
        var emptyFields = 0;

        $("#signup input").each(function () {
            if ($(this).attr("type") !== "password") {
                if (!$.trim($(this).val())) {
                    emptyFields++;
                }
            } else {
                if (!$(this).val()) {
                    emptyFields++;
                }
            }
        });

        if (!emptyFields) {
            // Disable button for 5 seconds to prevent subsequent submissions.
            $("#signup button").prop("disabled", true);

            setTimeout(function() {
                $("#signup button").prop("disabled", false);
            }, 5000);

            // Submit.
            return true;
        } else {
            // Dont submit.
            return false;
        }
    });
});

HTML

<form id="signup" method="post" action="" autocomplete="off" novalidate>
    <input type="hidden" name="W6K5Y" value="1xcSQoRqBOLu2NxPQhar">
    <div class="input">
        <label for="first-name">Full Name</label><!--
     --><input type="text" id="first-name" name="firstName" placeholder="First name" maxlength="35" autocomplete="off"><!--
     --><input type="text" id="last-name"  name="lastName" placeholder="Last name" maxlength="35" autocomplete="off">
        <div>
            <div class="addition">- ADDITIONAL MESSAGE</div>
        </div>
    </div>
    <div class="input">
        <label for="email">Your Email</label><!--
     --><input type="text" id="email" name="email" maxlength="254" autocomplete="off">
        <div>
            <div class="addition .error">- ERROR MESSAGE<</div>
        </div>
    </div>
    <div class="input">
        <label for="password">Password</label><!--
     --><input type="password" id="password" name="password" autocomplete="off">
        <div>
            <div class="addition">- ADDITIONAL MESSAGE</div>
        </div>
    </div>
    <div class="input">
        <div class="align">
            <label><input type="checkbox" name="terms">I AGREE</label>
        </div>
    </div>
    <div class="input last">
        <div class="align">
            <button>BUTTON</button>
        </div>
    </div>
</form>
\$\endgroup\$
7
\$\begingroup\$
  • Creating jQuery objects is not free. If you intend to use the same selector often, it's better to save it in a variable, like

    var $firstName = $("#signup input[name=firstName]");
    var $this = $(this);
    
  • In the first chunk, where you hide certain error messages by default, you're doing a very inefficient filter. There's a built-in .filter(selector) for that. Better yet, do the filter in the initial select:

    $("#signup .addition:not(.error)").css("display", "inline-block").parent().hide();
    

    I took out the "#signup div" because it seems like .addition will always be a child of #signup div. If that is not the case, or if you find it more readable to keep it, feel free to leave it in, but know that there's no functional difference.

  • input[name=firstName] is of type="text", and so calling .val() on it should always return a string. Thus, you don't need the first condition in $firstName.val() && $firstName.val().length <= 35.

  • To add to the previous point, you're using a magic number there. Consider assigning it to some locally-scoped constant, like var MAX_FIRST_NAME_LENGTH = 35;.

  • Rather than .parent("#signup div").siblings("#signup input"), it may be more extensible to use .closest("#signup div.input").children("input"). Then you don't need to worry about making sure you don't accidentally wrap an error in another <div>.

  • .stop()[e.type === "focus" ? "slideDown" : "slideUp"]("fast");

    This is incredibly opaque code. It took me more than a moment to realise that this is just a way to selectively call slideDown or slideUp. I'm not really sure how this can be rewritten concisely, but I wanted to point it out.

    • I also want to point out that it looks like if you have both a .addition and a .error in your #signup div, the additional info field will not hide.
  • Regarding your submit handler: earlier, you used the verbose but readable .length === 0. Why, then, are you using !$.trim(...) instead of $.trim(...) === ""?

  • I'm not a fan of iterating to check for empty fields twice. Instead, how about iterating through it once, using if (emptyFields++ === 0) { $(this).select(); }?


To expand on the last point: you use the following structure twice:

$("#signup input").each(function () {
    if ($(this).attr("type") !== "password") {
        if (!$.trim($(this).val())) {
            // some action here
        }
    } else {
        if (!$(this).val()) {
            // the same action
        }
    }
});

This could be DRYed out a bit. You can get rid of the first instance, and put the logic in the second instance like this:

$("#signup input").each(function () {
    if ($(this).attr("type") !== "password") {
        if (!$.trim($(this).val())) {
            if (emptyFields === 0) {
                $(this).select();
            }
            emptyFields++;
        }
    } else {
        if (!$(this).val()) {
            if (emptyFields === 0) {
                $(this).select();
            }
            emptyFields++;
        }
    }
});

This can be simplified to:

$("#signup input").each(function() {
    var $t = $(this);
    var val = $t.val();
    if ($t.prop("type") !== "password") {
        val = $.trim(val);
    }

    if (!val) {
        if (emptyFields === 0) {
            $t.select();
        }
        emptyFields++;
    }
});

For conciseness, you can replace the second if block with the following, if you think it's still acceptably readable:

if (!val && !emptyFields++) {
    $t.select();
}
\$\endgroup\$
0

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.