8: too many arguments, make this take a configuration object
10: why not attach view to the app object here?
1: remove the implicit globals here. if you want to make something global, assign it to window
6: this is why I hate handlebars
I understand wanting to separate the concepts of starting the application, but I would have done this differently. All of the views and stores seem like hard dependencies of the "app" module, and can be required without side effects. If that's the case, make them actual dependencies of the app module, passing them through needlessly is just misdirection.
why isn't this just using something out of the box from npm? uuid
or something similar. I see a lot of wheel reinvention here, considering that you can do this with bare node in a couple of lines
var cypto = require('crypto');
function createToken() {
var tokenRoot = crypto.randomBytes(30).toString('base64')
.split('/').join('_')
.split('+').join('-')
return crypto.createHash("sha1").update(tokenRoot).digest("hex");
}
random whitespace
22: assinging module.exports
to exports
is redundant. Either assign a single value to module.exports
or multiple properties to exports
. –– I guess this is from walmart?
1: same comment about assigning both module.exports
and exports
. also, creating the SettingsStore
variable is vestigial
19: implicit use of global jQuery
In general, why bother with "_private" method naming. None of this is a publically consumed API, and instance methods are never really private in JS... pointless naming convention, which is impossible to enforce and therefore will become tech debt over a long enough timeline.
I implemented a few of your suggestions with PRs in quickleft/tv. You feedback is much appreciate. I learn a lot from it. Keep it coming.
Hapi's style guide calls out using the "underscore" prefix for private members, so we should oblige. I'm glad I looked at it, because there's a few other things that we can square up.
Also, as you guessed, the
exports
style is a walmart thing too https://github.com/hapijs/hapi/blob/master/lib/connection.js#L30