Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Project Notes #35

Open
martypdx opened this issue Aug 19, 2016 · 0 comments
Open

Project Notes #35

martypdx opened this issue Aug 19, 2016 · 0 comments

Comments

@martypdx
Copy link
Contributor

(Feel free to close this issue, I just wanted to use markdown :)

Great project, really nice job managing things on the backend. Promise chains, models, routes, project org all really quite good.

Here are notes for suggestions I took while review the code:

  • Make sure not to check .env files into github. You might want to revoke/change they KEY if it might be bad for someone to get ahold of it.
  • Testing was good, would have liked to see more clear user auth API workflow tests.
  • Cache repeatedly accessed property values. For example:
    • here put a[i] into variable.
    • here put result.GoodreadsResponse.search[0] in variable
  • For root router paths, you can omit the path param. Instead of:
    .get('', (req,res,next) => {
    use:
    .get((req,res,next) => {
  • For Series get all, seems like you migh not have needed all the installment data. If it is needed, you still have duplication with the get by id, perhaps it should have been a model method populateInstallments?
  • Not sure why this route in the series module.
  • Looks like two ways to add a user, signup and users. Also, looks like I could modify password inadvertently using the users routes. Don't expose all data unless there's a need.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant