Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I took some time to create a function, which can validate which keys you input on an element. So far it has two uses:

  1. You call .validate() on the object, but make sure it has a data-match attribute with the regex in it
  2. You call .validate("REGEX") on the object, but replace REGEX with your actual regular expression

Here is an example:

document.getElementById("input").validate("[0-9A-Fa-f]");

This will only allow characters 0-9, A-F, and a-f. Nothing else.

I want to know, if I can improve this code. So far I have it the way I want it to be, but I would surely love someone else's feedback on it as well. It is really light-weight and can easily be implemented.

Thanks a lot!

Object.prototype.validate = function(myRegex) {
    var regex = this.getAttribute("data-match") || myRegex;
    var allowed = [8, 37, 38, 39, 40, 46];
    this.addEventListener("keydown", function(e) {
        var c = (e.metaKey ? '⌘-' : '') + String.fromCharCode(e.keyCode);
        if (!e.shiftKey && !e.metaKey) c = c.toLowerCase();
        if ((e.ctrlKey && (e.keyCode == 65 || e.keyCode == 97)) || c == '⌘-A') return true;
        if (c.match(regex) || allowed.indexOf(e.which) >= 0) {
            //Do nothing
        } else {
            e.preventDefault();
        }
    });
}

document.getElementById("input").validate("[0-9A-Fa-f]");
<input type="text" id="input" />

share|improve this question

Whatever you do DON'T MODIFY Object.prototype

Modifying Object.prototype is one of the worst things you can do in JavaScript

Quoting MDN:

Warning: Changing the [[Prototype]] of an object is, by the nature of how modern JavaScript engines optimize property accesses, a very slow operation, in every browser and JavaScript engine. The effects on performance of altering inheritance are subtle and far-flung, and are not limited to simply the time spent in obj.__proto__ = ... statement, but may extend to any code that has access to any object whose [[Prototype]] has been altered. If you care about performance you should avoid setting the [[Prototype]] of an object. Instead, create a new object with the desired [[Prototype]] using Object.create().


What should I do?

Preferably create a function:

function Validate(element, regex) {
    // code
}

but if you want to extend an element use Element.prototype, preferable with Object.defineProperty:

Object.defineProperty(Element.prototype, 'validate', {
    value: function(regex) {
        // code
    }
});

The bonus of using this is you can instead of getters and setters which are pretty cool too.

Avoid empty ifs

Here you have:

if (c.match(regex) || allowed.indexOf(e.which) >= 0) {
    //Do nothing
} else {
    e.preventDefault();
}

instead, use a NOT (!):

if (!(c.match(regex) || allowed.indexOf(e.which) >= 0))
    e.preventDefault();

the best way to write this specific condition would be:

if(!c.match(regex) || allowed.indexOf(e.which) < 0)

Use clear variable names

As much as I love code-golf (writing short code), this is not it. Make sure your variable names are clear.

I don't know what the c variable does so I can't suggest an alternative name, this also makes it more confusing to me on the workings of your code.

Use Regex literals not strings

Your example shows:

"[0-9A-Fa-f]"

That's a string, not a Regex, use Regex literals (to avoid escaping also):

/[0-9A-Fa-f]/
share|improve this answer
1  
Good answer overall. For the point about clear variable names, I think that the variable e is just fine; I find that it's quite common in JavaScript for e to be used as a parameter, especially when the method is an event listener. – SirPython 1 hour ago

Change.. all the objects!

No, don't do that. Downgoat already explains why. This will touch every object. I shouldn't be able to do this:

var foo = {
    bar: "hello world"
};

foo.validate(...);

Instead, if you want to modify prototypes, you should modify the prototype of HTMLInputElement. This will only touch input elements now, but you should still do some validation that the element is of type text.

HTMLInputElement.prototype.validate = function() {
    ...
}

Built-in pattern

Now, I don't know if you already know about this, but input tags already have a feature for validating input: pattern.

With pattern, you simply specify a regex and the form cannot be submitted unless that criteria is met. Now, this isn't exactly what you have: you have it so the user cannot even enter a wrong character. I personally don't think this makes a difference; after-all, most every input field should exist in some sort of form.

Here's an example usage:

<input type="text" pattern="([0-9]){3}"/>

Another good reason for using the built-in pattern would be that you won't have to do any extra meta key validation when the user is typing, in case you aren't handling all the possible cases. pattern is much more flexible.

share|improve this answer
1  
Do note that pattern support isn't perfect on iOS / Mac (though it's still pretty good on modern browsers) – Downgoat 1 hour ago
1  
Well, at least on Safari for iOS / Mac. But who uses Safari? (besides Alex) – SirPython 1 hour ago

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.