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.

Sign up for GitHub

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

Jump to bottom

Matt Cain submitting Custom API Homework #2

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Matt Cain submitting Custom API Homework #2

wants to merge 10 commits into from

Conversation

thrakc3001
Copy link

@thrakc3001 thrakc3001 commented Oct 10, 2017 •
edited
Loading

@jhnsnc
Copy link
Contributor

jhnsnc commented Oct 16, 2017

I like that you included notes to yourself in that Outline file. Often developers forget to include notes and other non-code artifacts like that. Sometimes developers will create a docs/ folder in their project for exactly this sort of thing.

I like how clean your routes/guitarists.js is. I do notice that you're passing along guitars in a very simple/direct way rather than looking through the guitars array and copying over specific properties. Note that this could be a weakness later on if you need to start verifying data in the guitar objects. Luckily you did define default values for guitars in the GuitaristSchema, so at least you won't end up with undefined properties.

A good way to expand on this might be to include a GuitarMaker/GuitarManufacturer model so you could have multiple guitars reference the same maker.

jhnsnc
jhnsnc reviewed Oct 16, 2017
View reviewed changes
server.js
// TODO: delete the above dummy routes and add your actual routes
const guitaristRoutes = require('./routes/guitarists');
app.get('/guitarists', guitaristRoutes.getGuitarist);
app.get('/:id', guitaristRoutes.getGuitaristById);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, I think you meant to make this endpoint GET /guitarists/:id rather than GET /:id

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhnsnc jhnsnc jhnsnc left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@thrakc3001 @jhnsnc