On Dec 1, 2012, at 13:31, Nick Italiano wrote: > I made a simple single room chat using express, mongoose, and socket.io. > If anyone is willing to give me some feedback or even a code review I would > really appreciate it. > > Here is the repo : https://github.com/unboundfire/wowel
Thanks for sharing your code! I ran it briefly and looked through the code and have the following feedback. Your package.json is not valid json (it's missing commas after some entries). The syntax highlighting in github shows this, as does the error message I got when trying to run "npm start". In package.json, you've listed "*" as some version numbers. You don't know how the authors of those projects will change the interface in the future, so you should understand what semver versions mean and how to specify a version number that will allow you to accept compatible upstream changes while avoiding potentially incompatible ones. http://blog.nodejitsu.com/package-dependencies-done-right In app.js, the variable name siteUrl is confusing, since it seems only to be used as the hostname of the mongodb server; you might change this variable name. In javascripts/index.js that variable name is used again but as the hostname of the web server, which seems like a more reasonable use for a variable of that name. But in javascripts/index.js you're only using it to connect back to socket.io, so you probably don't need to hardcode it; you can probably get the correct value out of the location object. Same for javascripts/login.js. In app.js, you probably shouldn't start listening on your server until after it's been set up (after the app.configure calls, and after the routes are connected). In app.js and javascripts/index.js, you have some cross-site scripting vulnerabilities because you're not making any attempt to escape msgs['msgs'][i]['msg'] or msgs['msgs'][i]['userid'] or sessionStorage.getItem('userId') or $('#myMsg').val() when concatenating htmlString. http://en.wikipedia.org/wiki/Cross-site_scripting Whenever you have a form with fields that have labels, like in login.ejs, each <input> element should have a unique id, and you should enclose the labels in <label> tags, and use the "for" attribute of the label tag to reference the corresponding input element's id. This way users can click the label to place the insertion point into the corresponding field. https://developer.mozilla.org/en-US/docs/HTML/Element/label Where you use the term "WhiteSpaces", it should not be plural, and the S probably should not be capitalized. http://en.wikipedia.org/wiki/Whitespace_character Where you require('mongoose/'), there's no need for there to be a slash at the end. Where you write require('./UserModel.js'), although that's fine, note that you don't need to specify the ".js" extension; you could just write require('./UserModel'). In models/ChatHistory.js, the addMsg function needs a callback parameter, which it should then call when the save has completed (or has failed with an error). You should use this to test for an error anytime you use the addMsg function. Same goes for addUser in models/User.js. I'm not sure why you've divided each model into two JavaScript files. Every other example I've seen has just one js file per model and that's worked fine for me. Of course you should not store the user's password in plain text. Use bcrypt or PBKDF2 or something. There's no validation on the email address. The binary files comprising the mongodb database itself probably don't belong in the repository. You have the schema in your model files; that should be enough to recreate the structure, and the data isn't needed. You probably don't need to store all chat history forever, in which case you might want to use a capped collection. http://mongoosejs.com/docs/guide.html#capped You might not need a separate timestamp field in your collection, since mongodb ObjectIDs already contain a timestamp. http://stackoverflow.com/questions/10370385/how-can-i-easily-access-the-mongodb-objectid-date-from-within-a-templating-engin -- Job Board: http://jobs.nodejs.org/ Posting guidelines: https://github.com/joyent/node/wiki/Mailing-List-Posting-Guidelines You received this message because you are subscribed to the Google Groups "nodejs" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, visit this group at http://groups.google.com/group/nodejs?hl=en?hl=en
