Yet Another Todo API

I've been on an adventure trying to learn NodeJs and put together a stack that I'd be happy to put into production if one day my team said to me "Let's build it in Node!". What we have here is your standard "Todo Webservice". Any and all feedback is appreciated, because I had never written more than 10 lines of JS prior to a month ago.

I do have a few specific concerns:

• The setup for the TodoControllerSpec feels kind of verbose. Can it be simplified?

• If I forget to call .catch((err)=> throw err) on a test, I can get silent failures. Is there a better way to handle unit tests when promises are involved?

• Have I structured my middleware in an expected way? This kind of felt right, but I'm unsure if there are any expected patterns for Express middleware to implement.

The "main" packages I'm using are Express, LokiJs, Mocha, and Chai, but for completeness, here's my package.json file so you know all the libraries I've imported.

"devDependencies": {
    "chai": "^3.5.0",
    "mocha": "^3.1.2",
    "node-mocks-http": "^1.5.4",
    "proxyquire": "^1.7.10",
    "sinon": "^1.17.6",
    "sinon-as-promised": "^4.0.2",
    "sinon-chai": "^2.8.0"
  },
  "dependencies": {
    "body-parser": "^1.15.2",
    "express": "^4.14.0",
    "handlebars": "^4.0.6",
    "hbs": "^4.0.1",
    "http-auth": "^3.1.1",
    "lokijs": "^1.4.1",
    "q": "^1.4.1"
  }

index.js

```
var express = require('express'),
bodyParser = require('body-parser');

var app = express();
app.use(bodyParser.json());
app.set('view engine', 'hbs');

var auth = require('http-auth');
var basic = auth.basic({file: __dirname + "/data/users.htpasswd"});
// uncomment to password protect the whole api
// app.use(auth.connect(basic));

// use this as middleware just for routes we want
var requireAuth = auth.connect(basic);

// require authentication for all post requests
app.post('*', requireAuth);

var root = require('./controllers/root'),
todo = require('./controllers/todo');

app.get('/'

Okay I'm going to be a bit more comprehensive here since I've already read your code quite a few times, and I'm gonna assume that you are on node v6


The setup for the TodoControllerSpec feels kind of verbose. Can it be simplified?

If you mean all the require calls you have to do...unfortunately not as far as I know. As a side note, I really like jest for this reason because it basically has almost all of your dependencies built into it, and additionally has built in code coverage and async support. You can see the API here.


If I forget to call .catch((err)=> throw err) on a test, I can get silent failures. Is there a better way to handle unit tests when promises are involved?

So once again the best way to handle this is to return your promise, and if there's a rejection, mocha itself will catch it, and fail. When I said that

.catch((err) => {
  throw err;
});

was useless, I meant that if you throw an error inside a catch block, it doesn't escape the scope of the promise; instead, it's just propagated to either the next catch block, or is left unhandled. Thus, the result of the above code, is the same as not catching at all.

This post I linked shows how you could make the error occur outside the promise, but I wouldn't recommend it.

Regarding chai-as-promised that's yet another dependency you'd need, and you can solve your problem without it.

Again, as a side note, in your actual application code, you use catch properly like here

.catch(function (err) {
  next(err);
});

instead of rethrowing the error, which is practically a no-op, you did something with it that wouldn't have happened otherwise.


Have I structured my middleware in an expected way? This kind of felt right, but I'm unsure if there are any expected patterns for Express middleware to implement.

Looks good to me :)

Other Points

Q, Deferred, and Promises

Don't use Q. First, it's not the best promise implementation, and second you're using it for the deferred anti-pattern.

Regarding the first point, ideally you don't require a promise library, and just use the built in Javascript promises. However, if you truly want to require another implementation, bluebird is the absolute best promise library there is, and even beats the promises that come by default in node both performance wise and in terms of having a more extensive API.

Regarding the second point, you're writing

exports.findById = function (id) {
    var deferred = Q.defer();

    db.loadDatabase({}, function () {
        try {
            var item = db.getCollection('todo')
                .findOne({ '$loki': id * 1 });

            deferred.resolve(item);
        }
        catch (err) {
            deferred.reject(err);
        }
    });

    return deferred.promise;
}

but the more idiomatic version is:

exports.findById = function (id) {
    return new Promise(function(resolve, reject) {
        db.loadDatabase({}, function () {
            try {
                const item = db.getCollection('todo')
                    .findOne({ '$loki': id * 1 });

                resolve(item);
            } catch (err) {
                reject(err);
            }
        })
    });
}

var vs. const vs. let

I only mention this because I see you wrote const testData in only one place, but nowhere else.

The long story short on the best practices I've seen is:

• Don't use var anymore because it's not block-scoped like let and const

• Use const for everything that you can. const only means that you can't reassign a variable using =. You can still mutate the variable if it's an object or array by adding/changing keys or pushing new values respectively.

• Use let when you need to use reassignment.

Lots more detail on this here

Conclusion

Your code looks mostly good, and other than some inconsistent style or extra syntactic sugar you could use, there's nothing else that objectively needs fixing.

Since you have a C# background, I would recommend you look into TypeScript. I wrote a blog post on a similar tool called Flow a while back that describes the benefits of why'd you'd want to use any tool similar to Flow/TypeScript.

I would also recommend you familiarize yourself more with the new es6 features that are coming out as of late. Here's a quick resource https://github.com/DrkSephy/es6-cheatsheet, and here's a comprehensive one http://exploringjs.com/es6/index.html#toc_ch_variables

Finally you might want to take some time to read through http://eslint.org/docs/rules/, and use eslint yourself. It's really helpful for understanding common mistakes people make and is a good tool to have overall.

StackExchange Code Review Q#150728, answer score: 7