current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite
2

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.

share|improve this question

2 Answers 2

up vote 3 down vote accepted

  • 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);".

share|improve this answer
    
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 – Prakash Aug 12 '14 at 9:39
up vote 1 down vote

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

share|improve this answer
1  
he can even reduce it to; return validChars.test(keychar) && keyChars.test(keychar) ? keychar : false; – renatoargh Aug 11 '14 at 20:20
    
I want to use backspace tab and tab key. How is it possible without /[\x00\x08]/ ? – Prakash Aug 12 '14 at 16:59

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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