Skip to content

Conversation

@sarahwalters
Copy link
Collaborator

Hey Ian, Jason, and James,

This looks great! Your code is unbelievably clean and well-organized, on the whole -- a couple comments throughout.

A few general points:

  1. Could you modularize your ./routes/routes.js file? I feel like a bit of reorganization would make the serverside request handling a little easier to understand & maintain... that's a >700 line file.
  2. Make sure you're not logging sensitive information (user email addresses, passwords, etc) -- particularly if you're planning to put this app to real use sometime in the future (which I'd love to see!)
  3. You've got some spacing/semicolon/etc inconsistency -- it's tricky to be consistent about those things when working on a team, but it's also (in my opinion) important for readability & maintainability. I pointed it out inline in a couple places, but not everywhere. A simple linter would help you catch that stuff :)
  4. Please make sure you're cleaning up your debugging mechanisms (console.log statements, etc) before you deploy/submit -- particularly because it sounds like you might want to pass this along to future student government teams to maintain/build on, I think you want to set a precedent of code cleanliness. You'll have kind of a tragedy-of-the-commons issue if every future contributor leaves their console.logs in your codebase.

Awesome work!

Remember not to merge these pull requests -- take a look at the feedback, feel free to comment if you'd like to discuss anything, and then close the PR.

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

Successfully merging this pull request may close these issues.

1 participant