3
\$\begingroup\$

I have this basic JavaScript that, using a constructor, validates the form where the login fields can't be empty. All this WITHOUT using any JS libraries.

function Validator(txtbox) {
    this.txtbox = txtbox;
}

var validator = new Validator([ "username", "password"]);

Validator.prototype.validate = function(form) {
    for(var i = 0, l=this.txtbox.length; i < l; i++) {
        var status = document.getElementById("status-msg");
        if (form[this.txtbox[i]].value == 0) {
            status.innerHTML="The " + form[this.txtbox[i]].name + " is empty";
            status.style.display = "inline-block"
            status.className = "error";
            return false;
        }
        else {
             status.innerHTML="Login successful";
            status.style.display = "inline-block"
             status.className = "success";
        }
    }
}

function runValidate(form) {
    validator.validate(form);
}

I'm wondering how I can make it more efficient or better.

\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

Why not add some JSDoc to your code:

/**
*@constructor
*@param {Array} txtbox - An array of whatever
*@returns {Validator}
*/
function Validator(txtbox) {
    /**
    *@private
    */
    this.txtbox = txtbox;
}

var validator = new Validator([ "username", "password"]);

/**
*@public
*@memberOf Validator.prototype
*@param {Form} form - The form holding the login information
*/
Validator.prototype.validate = function(form) {
    for(var i = 0, l=this.txtbox.length; i < l; i++) {
        var status = document.getElementById("status-msg");
        if (form[this.txtbox[i]].value == 0) {
            status.innerHTML="The " + form[this.txtbox[i]].name + " is empty";
            status.style.display = "inline-block"
            status.className = "error";
            return false;
        }
        else {
             status.innerHTML="Login successful";
            status.style.display = "inline-block"
             status.className = "success";
        }
    }
}

function runValidate(form) {
    validator.validate(form);
}
\$\endgroup\$
0
0
\$\begingroup\$

Your Validator(txtbox) function needs to change it's parameter name. It reads like it takes a single element, but in your case you are sending an array of elements.

All your validation does is just making sure there is something in the textbox, but i could still send a whitespace string into it, and if it was asking for my phone number there's no validating if it's a valid number, or not. Or maybe it's asking for my credit card and I send in my name. There's no validation for that.

I would also set it up so that the validation function takes the actual target element as parameter.

\$\endgroup\$
3
  • \$\begingroup\$ Many thanks for your answer. So a good name would be something like txtBoxArr maybe? Should I then check if the textbox is null? \$\endgroup\$ Nov 18, 2013 at 12:05
  • \$\begingroup\$ Sure, or if you decide to pass an array of elements I'd go with "elements" \$\endgroup\$
    – Max
    Nov 18, 2013 at 12:20
  • \$\begingroup\$ Ok, thanks again. When you say " actual target element as parameter" - how would I go about doing that? \$\endgroup\$ Nov 18, 2013 at 12:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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