Hi Ryan and Nick, > 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".
Yeah, that's why I usually make package.js and compile it into json later. Why aren't it a common practice? > 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 I'd point out that, while it might be a good advice for applications (with "~" semver operator), it's definitely a wrong one for public packages. Dependency means "should work with", not "known to work with". If some version of a package is excluded, it means that there is a certain reason why it is excluded, and this version should be placed in documentation somewhere. This way, "*" is fine unless you tested it and proved otherwise. Packages with locked dep versions effectively prevent to update software (see http://www.mikealrogers.com/posts/nodemodules-in-git.html), can cause multiple versions of the same package being used on the same system, etc. In other words, they just cause trouble. Don't do that. -- Regards, Alex 02.12.2012, 14:53, "Ryan Schmidt" <[email protected]>: > 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 -- 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
