3
\$\begingroup\$

My recent post had some things to fix. I think this should be well structured and written for you.

How does this code look to you? If something needs to be changed please tell me.

// Gets all elements by ID's
function elem(id) {
    return document.getElementById(id);
};
//


//Check box id"RadioGroup1_0" will be checked on start up of page (Phone)
window.onload = function () {
    document.querySelector("#RadioGroup1_0").click();
    //

//Gets my Form name (form) and on submission (submit) checks the rules below...

/* first name must be greater than 0
last name must be greater than 0
email must be greater than zero and follow the email reg ex
phone must be greater than 7 but shorter than 11
*/
    var form = document.getElementById('form');
    form.onsubmit = function (e) {
        var rules = [
        ['first-name', elem('first-name').value.length > 0],
        ['last-name', elem('last-name').value.length > 0],
        ['email', elem('email').value.length > 0 && /^([a-zA-Z0-9_\.\-])+\@(([a-zA-Z0-9\-])+\.)+([a-z0-9]{2,4})+$/.test(elem('email').value)],
        ['phone', elem('phone').value.length > 7 && elem('phone').value.length < 11 ]

    ];


   //This is the (reqMsg) error, it follows the ID's listed above above and if there is an error it must be "inline" and if there is no error it will be "none",     
        var valid = true;
        var firstFocus = null;
        for (var i = 0; i < rules.length; i++) {
            var parent = elem(rules[i][0]).parentNode;
            if (!rules[i][1]) {
                valid = false;
                parent.children[2].style.display = "inline";
                if (firstFocus == null) firstFocus = parent.children[1];
            } else {
                parent.children[2].style.display = "none";
            }

        }
          if (!valid) {
            firstFocus.focus();
            return false;
        }

        return true;
    };
};

// Using only alphabet keys NO Numeric for certain field boxes...
// First Name and Last Name
function onlyAlphabets(e) {
    try {
        if (window.event) {
            var charCode = window.event.keyCode;
        } else if (e) {
            var charCode = e.which;
        } else {
            return true;
        }
        if ((charCode == 64) ||
        (charCode == 32) ||
        (charCode >= 65 && charCode <=90) ||
        (charCode >= 97 && charCode <=122))
        return true;
        else return false;
    } catch (err) {
        alert(err.Description);
    }
}


/* isNumeric
*/

// This is for the phone field, only allowing NUMERIC numbers so no alphabets. It's referencing my above JS minimum 8 and maximum 10.

var specialKeys = new Array();
function IsNumeric(e) {
    var keyCode = e.which ? e.which : e.keyCode
    var ret = ((keyCode >= 48 && keyCode <= 57) || (keyCode === 8));
    return ret;
}
\$\endgroup\$
  • \$\begingroup\$ It seems like you fixed almost nothing :/ \$\endgroup\$ – Marc May 19 '14 at 11:48
  • 1
    \$\begingroup\$ To make life easier for reviewers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. See also this meta question. If your question is a follow-up, please see the guidelines to posting follow-up questions \$\endgroup\$ – Simon Forsberg May 20 '14 at 12:02
  • \$\begingroup\$ @Anonymous I've reverted your Rev 3. \$\endgroup\$ – 200_success May 20 '14 at 13:55
  • \$\begingroup\$ @200_success Is whitespace not superficial enough to silently fix? \$\endgroup\$ – Anonymous May 20 '14 at 14:49
  • \$\begingroup\$ @Anonymous If it bothered you enough that you wanted to change it, then it was worth mentioning in a review as a side note. (Feel free to follow up on that Meta post or on chat.) \$\endgroup\$ – 200_success May 20 '14 at 14:55
6
\$\begingroup\$

Validation problems

Beware of overzealous validation. It's easy to create bugs by rejecting valid input. For example:

  • Some people have only one name, so the one of the name fields must be allowed to be empty.
  • Names often contain non-ASCII characters. Validation must not tell the user their name is invalid!
  • Email addresses can contain more characters than you think. For example, "Hello, world!"@localhost.localdomain is a valid address. The validity rules are complex, so if you try to implement them with a regex, you will get it wrong. Fortunately you don't need to reject all invalid addresses, only the most common mistakes. So /@/ is enough validation for practical purposes.
  • Phone numbers can be long, because of country codes and extensions. They can also contain characters like +-()x.

You can avoid these problems by not doing much validation. Don't do unnecessary work that creates bugs!

I wouldn't bother with the event filters at all, especially for the name, where any character should be accepted.

Style

onlyAlphabets should be called onlyLetters (it allows letters, not whole alphabets!), or onlyNameChars, since it also allows some other characters.

e.which ? e.which : e.keyCode can be simplified to e.which || e.keyCode.

This conditional...

if ((charCode == 64) ||
    (charCode == 32) ||
    (charCode >= 65 && charCode <=90) ||
    (charCode >= 97 && charCode <=122))
    return true;
    else return false;

...can be simplified to:

return charCode == 64 ||
       charCode == 32 ||
       charCode >= 65 && charCode <= 90 ||
       charCode >= 97 && charCode <= 122;

...but character codes are hard to read. It might be better to use the string form instead, via operations like String.fromCharCode and charCodeAt.

There are too many blank lines. It's OK to leave one blank line to separate sections (like a paragraph break), but (like anything that consumes lines) they reduce the amount of code that fits on one screen, they should be used only when they improve clarity enough to be worth the extra scrolling.

\$\endgroup\$
  • \$\begingroup\$ There will always be a first and a last name, at least in western culture, so your point on the name-validation is somewhat moot. While you may be right that many people do not have a middle name, one can assume they will have at least a firstname and a lastname, meaning both fields should be required. \$\endgroup\$ – Vogel612 May 20 '14 at 9:38
  • \$\begingroup\$ @Vogel612 Only 25% of people are from western culture. Single names are common in Indonesia, for example. \$\endgroup\$ – Anonymous May 20 '14 at 13:52
3
\$\begingroup\$

Just a short remark on some of your if-statements and rules:

if ((charCode == 64) ||
(charCode == 32) ||
(charCode >= 65 && charCode <=90) ||
(charCode >= 97 && charCode <=122))
return true;
else return false;

You should always place curly braces when separating blocks. Apple f*cked it up and look what they got. The whole programming world laughs ;)

But that's not all. As you just return the result of your if statement in the corresponding blocks, why not return the result in the first place?

return (charCode == 32 || (charCode >= 64 && charCode <= 90)
   || (charCode >= 97 && charCode <=122));

Does exactly the same... (Thanks to @200_success for finding my error in the prev. version of this answer)


['first-name', elem('first-name').value.length > 0],
['last-name', elem('last-name').value.length > 0],
['email', elem('email').value.length > 0 && /^([a-zA-Z0-9_\.\-])+\@(([a-zA-Z0-9\-])+\.)+([a-z0-9]{2,4})+$/.test(elem('email').value)],
['phone', elem('phone').value.length > 7 && elem('phone').value.length < 11 ]

You are not checking the phone number for being only numeric. Also if you include country codes in your phone number things will break when using your IsNumeric(e) on the phone field, but that depends on your requirements ;)

When including country code, you could instead validate against something like this regex:

/^(\(?\+?[0-9]{2,3}\)?)?[0-9]{7,11}$/

Btw, your rule currently also does neither allow 7 nor 11 digits :(

\$\endgroup\$
  • \$\begingroup\$ Hi there Vogel thanks for the feedback. What I've done in my HTML for the fieldbox doesn't allow more then 10 digits, (maxlength = 10). And in the bottom I have isNumeric for the phone box. \$\endgroup\$ – Salata May 19 '14 at 10:47
  • \$\begingroup\$ @Salata as far as I remember, phone numbers may have up to eleven digits. Also given you want to go for international numbers, there's a few different allowed notations. Taking germany as example: (+49) 123 4567 890, +49 123 4567890, 0049 123 456 7890 are all valid phone numbers. I couldn't even enter the first two in your phone box :( \$\endgroup\$ – Vogel612 May 19 '14 at 11:00
  • \$\begingroup\$ I didn't allow (+) or what not just straight numbers :p \$\endgroup\$ – Salata May 19 '14 at 11:28
  • \$\begingroup\$ @Salata then that part of my answer doesn't concern you.. what a pity. Well it's a PITA to validate phone numbers either way, soo... \$\endgroup\$ – Vogel612 May 19 '14 at 11:49
  • \$\begingroup\$ You don't even need an if-else. Just write return (charCode == 32 || (charCode >= 64 && charCode <= 90) || (charCode >= 97 && charCode <= 122)); \$\endgroup\$ – 200_success May 20 '14 at 11:33

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.