Re: [nodejs] First node.js project would anyone be willing to review my code and give me some feedback?

[Ryan Schmidt]

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

[Alex Kocharin]

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" <googl...@ryandesign.com>:

> 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

> --
> 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 nod...@googlegroups.com
> To unsubscribe from this group, send email to
> nodejs+un...@googlegroups.com
> For more options, visit this group at
> http://groups.google.com/group/nodejs?hl=en?hl=en

[Nick Italiano]

Thanks for the feedback guys.  

I made a change to my package.json before I pushed it to git and of course I didn't check it before posting, lesson learned.

With the line require('mongoose/') thats the only way it seems to find the module on my localhost in Windows 7, but when I run it on amazon ec2 with a linux ami it can find the module with require('mongoose').

I'll be reading all the links and make my project more complete.

Again thanks for the help.

On Saturday, December 1, 2012 2:31:10 PM UTC-5, Nick Italiano wrote:

Hey,

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,

Nick