Skip to content

Grading and Feedback PR [DO NOT MERGE]#77

Open
martypdx wants to merge 2 commits into
RealTeamCtrl8:masterfrom
martypdx:master
Open

Grading and Feedback PR [DO NOT MERGE]#77
martypdx wants to merge 2 commits into
RealTeamCtrl8:masterfrom
martypdx:master

Conversation

@martypdx
Copy link
Copy Markdown

@martypdx martypdx commented Nov 8, 2017

Great choice for a mid-project and a lot of good work here!

  • > npm i && npm test passed on first try 😄 👍

  • Largely consistent coding style - look like from same person 😄

  • Prioritize most critical parts of the app, like running races. You only picked a random winner, vehicle speed and distance never came into play. Running races should have been one of the first things you completed

  • Need to enforce user name/email uniqueness, pretty much broken if two users choose same name.

  • Need to identify parallel actions and use Promise.all. Seems to be lacking across the board. You are responsible for orchestrating workflows correctly.

  • Need to refactor your code. Lots of gross duplication, see example refactorings

  • Use model methods and statics

  • Delete unused files

  • Reduce scope of recreating seed data (don't do in a beforeEach). Options:

    • use a before
    • create seed data in global before and only drop collections in individual tests
    • export seed-data and move to using a mongoimport to load
  • Don't put whitespace inside of .then(

  • Variable names

    • Prefer domain abstractions, got and found are substitutes in tests when they make sense, not reason to avoid naming. Especially when obscures singular vs plural nature of data value
    • No data types in variable names
  • Use model find and save when appropriate

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