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

Grading PR #150

Open
wants to merge 3 commits into
base: development
Choose a base branch
from
Open

Grading PR #150

wants to merge 3 commits into from

Conversation

martypdx
Copy link

Great first project, it presented some challenging tracking and math problems you solved well.

Overall good consistent team code style.

Biggest thing to work on going forward is refactoring and modularization. Routing logic, while functional correct, was not being refactoring into common functions including moving logic to models.

  • Only had model tests for user with basic require. Other models and interesting instance methods that should have been tested
  • Workflows should not be serial when they don't need to be
  • Not sure why stockStore was a single document vs a collection
  • Don't expose routes that are never used (stockStore)
  • Routes would be much cleaner had model logic been moved into the model modules
  • Generally, variables should be declared in narrowest scope possible. Several instances of variables being much broader than needed.
  • I coded some examples in portfolio.api.test.js and some of the routers of refactoring code into more DRY and logical composition. Keeping test code clean encourage writing more tests.
  • Would have been better to use setInterval on your server to periodically update stock rather than relying on external trigger which also necessitated exposing additional http api's
  • Put static assets to be served into folders and only expose those via express.static. Serving your root is bad idea, I can look at all your server resource. Imagine if you had an .env file...
    image

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