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

Form Validation Evaluation

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

Problem

I've been trying several styles of validation code for a form. I was wondering if this is good or needs some improvement or just trash it and use jQuery validator code?

$("#registration-form").on("submit", function(e) {

    e.preventDefault();
    var error_field = "";

    $(this).find(".input-field").each(function() {

        var field_id = $(this).attr("id");
        var value = $.trim($(this).val());
        var length = value.length;
        var error_value = "";

        // Do field checks
        if (length == 0) {
            error_value = "You missed this field. D:";
        } else if (length > 0 && length  30 && field_id == "username") {
            error_value = capitalize(field_id) + " too long";
        } else if (length > 50 && field_id == "password") {
            error_value = capitalize(field_id) + " too long";
        } else if (passwordCombination(value) === false && field_id == "password") { 
            error_value = capitalize(field_id) + " must contain at least one string, number, and symbol.";
        } else if (emailValidate(value) === false && field_id == "email") {
            error_value = "Invalid email! :(";
        }

        // Do show error labels
        if (error_value != null) {
            $("#registration-form .content-row."+ field_id + " .box-error").text(error_value);
            error_field += error_value;
        }
    });

    if (error_field == "") {

        var username = $.trim($("#username").val());
        var email = $.trim($("#email").val());
        var password = $.trim($("#password").val());

        $.ajax({ type: "POST", url: "process/app_usage.php",
            data: { username: username, email: email, process: "validate_user_email" },
            success: function(data) {

                // Do stuff
            }
        });
    }
});

Solution

I was wondering if this is good

Well, are you just looking to prevent the normal user from messing up during registration? I think it would be fine for that.

I'll see if I can point out flaws in your validation...

  • First, you check to see if length is 0. If it is, you warn them. Cool. But right after, you check to see if it's greater than 0. This is redundant? It would have to be longer than 0 characters to pass the first conditional!



  • Now you check length



  • Why can't the username be longer than 30 characters? Is your database not capable of such long strings? If it truly just cannot handle those awfully long names, fine. If it can, I'd suggest you support long names! Besides, I'd hate to find a new username if I've always used my most favorite 32 characters username!



  • Why can't my password be longer than 50 characters either? I might be one of those users who has funky passwords that are always has 9 words in their passwords.



  • What exactly is passwordCombination()? If I'm one of those funky users I mentioned in point 4, I'd be upset with this rule.



  • Use to validate emails on the client-side, not emailValidate(). Then check it again on the server with something such as PHP's filter_var().



  • ":(" should really be "):" to be consistant with eye placement in "D:".



There's some critique on your validation. So maybe it would be best to find an established validation library and make slight alterations to suit you! Google has some validation standards, and W3C also has some tips and tricks up their sleeve!

But always remember:


Servers should not rely on client-side validation. Client-side validation can be intentionally bypassed by hostile users, and unintentionally bypassed by users of older user agents

Now I'm not JavaScript maniac, but I might be able to help a little!

  • I believe you could generalize $(this).find(".input-field") into something such as $(this).find("input[type=text], input[type=password]"). This way you don't rely on having that class.



  • You just check if error_field` is empty, so why not make it simpler and turn it into a counter. Each error increments it. In the end, if it's value is equal to 0, it's all good!

Context

StackExchange Code Review Q#48453, answer score: 6

Revisions (0)

No revisions yet.