Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote -2 down vote favorite

I couldn't find any adequate-seeming email address validators, so I decided to write my own. This should be useful in Node.js and the browser. However, it's my first time doing such a thing, so I'd like to get some reviewage, if anyone doesn't mind. Cheers! http://dpaste.org/urMoa/

share|improve this question
3  
Please put the code in the question – AD7six Nov 24 '12 at 8:55

closed as not a real question by Corbin, Glenn Rogers, palacsint, Brian Reichle, Jeff Vanzella Nov 26 '12 at 3:25

It's difficult to tell what is being asked here. This question is ambiguous, vague, incomplete, overly broad, or rhetorical and cannot be reasonably answered in its current form. For help clarifying this question so that it can be reopened, visit the help center.If this question can be reworded to fit the rules in the help center, please edit the question.

1 Answer

up vote 2 down vote

Some thoughts, they're not all going to be gold:

  1. str.replace(/^\s*|\s*$/g, '') is the same as str.trim()
  2. if(str.match(/@/g).length !== 1 || str.match(/^@|@$/g)) is roughly the same as str.match(/[^@]@[^@]/).length !== 1 (not sure which you want for code clarity)
  3. isPurelyAscii(str)- couldn't this just be /^[ -~]+$/g.test(str), it's only called once
    • You use str.match(regex) a lot where regex.test(str) would seem to make more sense
  4. Any reason why your inner function are at the bottom? It seems you really don't like Crockford... I guess this is just a style thing though...
  5. Your regexes should probably be created in an outer scope so they're not recompiled each time the function is called. I'm not sure if javascript engines optimize for this, but it could be a performance issue.
  6. if(!str) return false; should probably be if (typeof str !== 'string'), otherwise all of your str.match() calls would throw exceptions. This is a non issue if you're sure a string will always be passed (not a buffer or anything like that).
  7. Why not use Array.prototype.every for the loop in lines 90-92? It does the same thing and is a little more clear on what's going on.
  8. isValidDNSLabel- why not do return !/^-|-$/g.test(str) instead?

It seems you're trying to export only a single function, which is why you use those inner functions instead of having them be stand-alone. If performance is not an issue, then this is totally fine, but if this is going to be run a lot on a server, then you might want to think about bringing those regex objects outside of the function and declaring them somewhere where they won't get recompiled each call. For client code, not a big deal though.

All in all, the code looks pretty clean though. I didn't really look it over for correctness though, but this can and should be part of a trivial unit test.

share|improve this answer
Thanks for all the thought you've given my code!! I'm not used to Array.prototype.every, would this, on line 90, be the correct way to use it? return labels.every(isValidDNSLabel) ? true : false; However, wouldn't this as well as subbing in String.prototype.trim reduce browser compatibility? I've never written code specifically for the server, but how high-traffic of a site does one need to have before needing to worry about such things as scalability and performance? Yeah, I'm not a huge fan of Crockford. I like some of the "bad" parts, and function-hoisting is one of them.... – wwaawaw Nov 24 '12 at 10:16
...I think it makes for far more readable code. Regarding isPurelyAscii, I think having it in its own function gives more space for annotating it with explanatory links and comments. Naming the function also allows for more fluent reading of the code, because it provides a semantic label, even if it abstracts the regex itself away more. For #2, I like my way better, but thanks for the suggestion, all the same. Here's the new version, BTW: dpaste.org/yGYhU – wwaawaw Nov 24 '12 at 10:26
Also, I just found a bug with it... See here – wwaawaw Nov 24 '12 at 10:38
2  
Array.prototype.every returns a boolean, just do return labels.every(isValidDNSLabel). As for browser compatibility, I would extend String.prototype and Array.prototype with the implementation on MDN for trim and every. – tjameson Nov 24 '12 at 19:05
1  
Concerning server performance, it really depends on how often the code is being called. It's always nice to throw in minor optimizations when they don't cost any readability, plus, one could argue that it's more readable to move the regexes out and give them a name. This allows the code to be self documenting (I prefer a clear, concise name to a comment). – tjameson Nov 24 '12 at 19:09
show 1 more comment

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