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

Feedback #1

Open
ajbraus opened this issue Feb 26, 2019 · 1 comment
Open

Feedback #1

ajbraus opened this issue Feb 26, 2019 · 1 comment

Comments

@ajbraus
Copy link

ajbraus commented Feb 26, 2019

  1. Very professional README.md - Excellent
  2. Love your clean and minimalist design. If I had one criticism it is careful to not be too sparse. Information density is very important for web design.
  3. You can use {timestamps: true} to have mongoose handle createdAt and updatedAt for you automatically.
  4. "checklistId" is more conventionally just => "checklist". But either way :D
  5. Instead of "completed" as a boolean, make it "completedAt" and make it a datetime. Then you have both if it is completed and when in one piece of data.
  6. Careful with "todoItems", this should be => "todos" and be an array of Schema.type.ObjectId.
  7. Same for "OwnerUserId" => "owner" or even just "user" to be clearer.
  8. reminder to gitignore your node_modules
  9. You require bcrypt in your app.js but don't use it. so you can just remove it. Only require things you use. YAGNI
  10. I really like your code. So clean, and you use one-liners the same way I do.
  11. Review and use const, var, and let consistently and correctly. Lots of extra var's around.
  12. Your controller code could be cleaner though, imo, put more on one line.
  13. If you fix your naming of your associated models you can use populate() to clean up your queries in your controllers.
  14. Where are your socket calls?
  15. If you use .sort() make sure to chain it and then finish with a .then() like this: .find().sort().then().catch()

Main things are to clean up your naming of attributes, timestamps, and especially the naming of associated resources. I'd also like to see the sockets but couldn't find. Otherwise looking good.

@nsafai
Copy link
Owner

nsafai commented Feb 27, 2019

Thank you for the super insightful and actionable feedback @ajbraus - also really appreciate the recommendations. I just looked up populate() and it looks great!

I haven't replaced current GET / POST requests with sockets yet, but planning on doing so shortly.

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

2 participants