5
\$\begingroup\$

This is a function for JavaScript validation:

var alpha = "[ A-Za-z]";
var numeric = "[0-9]"; 
var alphanumeric = "[ A-Za-z0-9]"; 

function onKeyValidate(e,charVal){
    var keynum;
    var keyChars = /[\x00\x08]/;
    var validChars = new RegExp(charVal);
    if(window.event)
    {
        keynum = e.keyCode;
    }
    else if(e.which)
    {
        keynum = e.which;
    }
    var keychar = String.fromCharCode(keynum);
    if (!validChars.test(keychar) && !keyChars.test(keychar))   {
        return false
    } else{
        return keychar;
    }
}

and HTML code:

<input type="text"  name="shipname"  onkeypress="return onKeyValidate(event,alpha);"/>
<input type="text"  name="price"   onkeypress="return onKeyValidate(event,numeric);" />

I want to know about code quality, creating issues or any alternative for this. I need some suggestion from experts.

\$\endgroup\$
0

2 Answers 2

6
\$\begingroup\$
  • onKeyValidate is an okay name, but a better name could be validateKeypress.

  • It seems very silly to store a RegExp as a string, and then construct it every time. Why not just declare var alpha = /[ A-Za-z]/?

  • keyChars appears to check against \x00, the null character, and \x08, the backspace character. Neither of these can ever be passed to onKeypress, so you can just take it out.

  • The standard way to get the character code is event.which || event.keyCode.

  • event is a global; I don't think you need to pass it in.

Here's a proposed rewrite:

var alpha = /[ A-Za-z]/;
var numeric = /[0-9]/; 
var alphanumeric = /[ A-Za-z0-9]/;

function validateKeypress(validChars) {
    var keyChar = String.fromCharCode(event.which || event.keyCode);
    return validChars.test(keyChar) ? keyChar : false;
}

The HTML will have to change to onkeypress="validateKeypress(alpha);".

\$\endgroup\$
1
  • \$\begingroup\$ This is not working for me. onkeyPress="return validateKeypress(event,alphanumeric);" and javascript : function validateKeypress(e,validChars) { var keyChar = String.fromCharCode(e.which || e.keyCode); return validChars.test(keyChar) ? keyChar : false; } This only working for me \$\endgroup\$
    – PNG
    Aug 12, 2014 at 9:39
1
\$\begingroup\$

The thing that I was able to pick out, and it's more of a nitpick type of things is that you should turn your last if statement around

if (!validChars.test(keychar) && !keyChars.test(keychar))   {
    return false
} else{
    return keychar;
}

should look like this

if (validChars.test(keychar) && keyChars.test(keychar)) {
    return keychar;
} else {
    return false;
}

Do your Positive first. most people like this better than all the negatives.

Side Note: for code golfing you just shaved 2 characters as well as made it more standard compliant if this nitpick can be considered a standard.

Short Version:

If you know Ternary operators and would like to use them instead of this simple if statement, @renatargh mentioned that you could make this super short

return validChars.test(keychar) && keyChars.test(keychar) ? keychar : false;

Also, var alphanumeric = "[ A-Za-z0-9]"; is never used (in this code block) and neither is var keyChars = /[\x00\x08]/;

you should just get rid of them

\$\endgroup\$
2
  • 1
    \$\begingroup\$ he can even reduce it to; return validChars.test(keychar) && keyChars.test(keychar) ? keychar : false; \$\endgroup\$ Aug 11, 2014 at 20:20
  • \$\begingroup\$ I want to use backspace tab and tab key. How is it possible without /[\x00\x08]/ ? \$\endgroup\$
    – PNG
    Aug 12, 2014 at 16:59

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.