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
up vote 5 down vote accepted

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 Mar 13 at 3:53
    
Thanks a lot for your comment! First of: This is going to be used on an element, which is why I modified Object.prototype. That seems to be the only way, I can create a .validate() function. Second of: The empty if-statement doesn't really work for me. I was originally going to leave out the //Do nothing but I couldn't figure out how to actually do it. Neither could you, because it doesn't work. Third of: Okay, I'll name my c variable char, even though c is a common abbreviation for character. Fourth.. of?: Yes this would work better, even though it's just a set of characters. – Snorlax Mar 13 at 10:50
    
Oh and also, the empty if would generally be used for a "Hey you entered an invalid character"-tooltip, so it won't just ignore them. – Snorlax Mar 13 at 11:00
    
@Downgoat Concerning the usage of ifs: Isn't it harder to think the way (NOT someState === true) ? I'm not sure ... but (someState === true) feels more natural to me. Might be a personal preference ... What's the exact benefit of write it the suggested way? – st88 Mar 13 at 12:40

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 Mar 13 at 3:52
1  
Well, at least on Safari for iOS / Mac. But who uses Safari? (besides Alex) – SirPython Mar 13 at 3:54
    
1) I COULD do HTMLInputElement but I wanted it to be as "versatile" as possible. So I want it to work on <textarea> elements as well, for instance. 2) Yes, I know about pattern, but it doesn't work for this type of application unfortunately. You can still enter a wrong character, but it will give you an error message when you defocus the element. Mine simply takes a set of characters, then validates if the entered character is legal. Something you cannot do with the new HTML5 attributes. – Snorlax Mar 13 at 10:53

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.