I realize criticism is cheap, and I like seeing small projects like this to explore other people's mind, but:<p>What's the reason for the valid_url_pattern in shortener.js?<p>/https?:\/\/([-\w\.]+)+(:\d+)?(\/([\w/_\.]*(\?\S+)?)?)?/<p>Why? This seems to be broken by design (I can construct invalid urls that match and find valid urls that don't. What's the idea behind this expression, why is it necessary?<p>And looking at the code it just seems to prepend '<a href="http://" rel="nofollow">http://</a> to the url if it doesn't match this expression?
Having a quick read through the code (so I might have missed something) but the URL is generated from a substring of an SHA hash, does this not mean, at least in theory, URLs could be duplicated?<p>With a hex hash at 6 chars, there are a possible 16,777,216 unique URLs (Probably not an issue, unless you're bit.ly). But when there have been 1,677,721 URLs generated, there is a 10% chance a URL will be duplicated, at 8,388,608 there is a 50% chance, and so on. While those numbers are very high and probably not an issue, it's worth considering this probably wouldn't work on a large scale (Though I'm sure it wasn't designed too)<p>Not really a criticism, I still think this is a great example of how simple Node is, just an observation
I'm new with Node.js, but the "shorten" process shouldn't be an asynchronous process? Maybe something like this?:<p><pre><code> app.get('/shorten', function(req, res) {
console.log('Shortening url...');
shortener.shorten(req.param('u'), function(result){
res.send(result);
});
});
</code></pre>
It's well known that Redis is pretty fast, but, citing "Node Web Development" (<a href="http://www.amazon.com/Node-Web-Development-David-Herron/dp/184951514X" rel="nofollow">http://www.amazon.com/Node-Web-Development-David-Herron/dp/1...</a>) on it "Architecture: Threads versus asynchronous event-driven" chapter, <i>"(...)Depending on the query that pause can be quite long. This is bad because while the entire thread is idling another request might come in, and if all the threads are busy it will be dropped. Looks like quite a waste. Context switching is not free either, the more threads we use the more time the CPU spends in storing and restoring the state. Furthermore, the execution stack for each thread takes up memory. Simply by using asynchronous, event-driven I/O, Node removes most of this overhead while introducing very little on its own."</i><p>It is not a criticism but a question.