[x] Run npm run jscs. This checks the code against Google's style guide, which we pretty much adhere too. There are a bunch of issues at the moment, and you'll get a list of them from jscs
[x] We have two instances of the same test here and here
[x] Write simple unit tests for the search and pagination services
[x] In Express v4, the canonical way to set routes is via the Router. This is not a biggie, but I was first confused by the use of the custom routes.configure
[x] When I see index.js in a module I would expect it to be the entry point into the module. However, here in the controllers it is not the entry point, but rather the controller for the home page. Again, not a big issue at all.
[x] In controllers, we have a file for each "page". I'd rather see controllers and routes grouped by "type". So we might have one file called pages that has all the generic stuff, like about, contact, and whatever else we'll add, errors for the various error responses, and another one called trials that has the "list" and "detail" controllers for trials
[x] Don't commit the bundled files (eg: app.min.js and vendor.min.js) to repo
Note: Not all of these require that you change the code. We can chat about them and decide what should be changed, and what is ok as is. At very least, you must do the first three points, and then we can talk on the rest.
Note: I also changed some stuff in gulpfile.js and package.json, as we need to keep the build stuff out of the code we commit. I did this because I am trying to work out for myself the best way to do this and to deploy to heroku. Not sure we are fully there yet as I had issues with incompatabilities between my filesystem and that of Heroku with case sensitivity, but, what I did do is remove all the built files and run them on install to Heroku via the npm run postinstall task.
pwalsh
commented
9 years ago
Notes from an initial code review.
npm run jscs. This checks the code against Google's style guide, which we pretty much adhere too. There are a bunch of issues at the moment, and you'll get a list of them fromjscsroutes.configureindex.jsin a module I would expect it to be the entry point into the module. However, here in the controllers it is not the entry point, but rather the controller for the home page. Again, not a big issue at all.pagesthat has all the generic stuff, like about, contact, and whatever else we'll add,errorsfor the various error responses, and another one calledtrialsthat has the "list" and "detail" controllers for trialsapp.min.jsandvendor.min.js) to repoNote: Not all of these require that you change the code. We can chat about them and decide what should be changed, and what is ok as is. At very least, you must do the first three points, and then we can talk on the rest.
Note: I also changed some stuff in
gulpfile.jsandpackage.json, as we need to keep the build stuff out of the code we commit. I did this because I am trying to work out for myself the best way to do this and to deploy to heroku. Not sure we are fully there yet as I had issues with incompatabilities between my filesystem and that of Heroku with case sensitivity, but, what I did do is remove all the built files and run them on install to Heroku via thenpm run postinstalltask.