Skip to content

Instantly share code, notes, and snippets.

@wookiehangover
Last active August 29, 2015 14:11
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save wookiehangover/200d8679f3f19bbfd6c3 to your computer and use it in GitHub Desktop.
Save wookiehangover/200d8679f3f19bbfd6c3 to your computer and use it in GitHub Desktop.
tv code review – part 1

source/js/app.js

8: too many arguments, make this take a configuration object

10: why not attach view to the app object here?

source/js/start.js

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.

source/js/clientIdGenerator.js

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?

source/js/settingStore.js

1: same comment about assigning both module.exports and exports. also, creating the SettingsStore variable is vestigial

source/js/views/app.js

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.

@mikeabiezzi
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment