Tinusw/TodoList — pull request #1 by skattyadz (2013-11-19 12:44:21+00:00)
Title: Feedback

Hey. I haven't run it yet, but after a brief look at your writeup and a flick through the code, here's what I wrote down :)

### Jade
Jade still has syntax, but it uses whitespace to remove the syntax that doesn't add much. Closing tags are gone, and if you start with . or # is assumes a class or id on a div, which is very common nowadays. IMO if you're doing code right, you're indenting anyway, so little's lost by using it.

### Node
The power in Node is the callbacks and module encapsulation. Callbacks are weird. Anything asynchronous has to return data in a callback function, as it won't happen inline with the rest of the code (hard to explain in text in a hurry!). You'll come to understand the implications of this

### Mongo
Mongo doesn't have incremental IDs (0,1,2,3,4,5), but instead has IDs combined of timestamp, machine number, cluster, etc. This is super epic, because it means clients can create new documents with unique IDs without having to get incremental IDs from the database server. They're still sortable, if you sort them they'll be in creation order to the nearest second. Note that Mongo also has 'natural order' which is slightly different (google it).

A Mongo collection holds 'documents'. That literally means some data in whatever format or structure you want. Collections don't have a defined schema, so if you want to start inserting things with an extra column, it makes no difference.

You can query on any column very fast, despite the fact that there isn't a schema - this is awesome. Technically if you're building something to scale, you'll want to index those field names, but don't worry about that for now.

Write concerns are an important part of mongo that often get ignored. If you want, have a quick google. Often the mongo defaults are 'fire and forget', which gives fast performance at the cost of robustness

### Brief look at code
Main file is generally ok. Some things that could be nitpicked, but it's mostly express boilerplate :)

provider file:

https://github.com/Tinusw/TodoList/blob/master/todoprov.js#L56
Best to use braces if your code inside the else block is more than one line. It might work in this case, but it's likely to give you unexpected behaviour somewhere along the line.

Also, this bit it indented differently, so I'm guessing it's from the interwebs? :P. I'm a bit of a code style nazi.

todo_collection.db.bson_serializer.ObjectID.createFromHexString(id)

Kinda messy, but not a massive problem. Maybe provide a convenience function that does this.

Personally I'd do:

var ObjectID = require('mongodb').ObjectID;

at the top of the file, then you can use:

new ObjectID(idString);

You're not using the callback for when the database has opened. This is fine for now, but if you tried to do any operations when the server starts, you might find the DB hasn't been connected yet.

In each of your database methods, you're getting the collection again. You should only need to do this once, then you can call things on that collection again and again. I've knocked up how this might look in the attached diff

`todoprov.prototype.save` isn't amazingly named. `todoprov.prototype.insert` would make its purpose a lot clearer

That's it for now. As I said, haven't looked at the running code yet :)

I've attached some changes that would reuse a single collection object for your calls, just because it's a bit tidier. It should also give it a new callback, so the calling code knows when the database has been loaded. This is in the form of a pull request, but don't accept or merge it. It's simply so you see a diff.