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

Project Express API by Anna #520

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

Conversation

Anna2024WebDev
Copy link

Copy link
Contributor

@HIPPIEKICK HIPPIEKICK left a comment

Choose a reason for hiding this comment

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

Good job Anna! Just remember to keep it clean and RESTful until next time :)


## The problem

Describe how you approached to problem, and what tools and techniques you used to solve it. How did you plan? What technologies did you use? If you had more time, what would be next?
I looked a lot at Matildas live session where she had different examples of query parameters or route parameters and filtering or find methods. I think it was very straight forward but I had one problem with the find method and route paths, there was a conflict between the routes for breed and ID so I needed to add more unique path to the cats breed. "/cats/breed/:breed" instead of "/cats/:breed" which worked.
Copy link
Contributor

Choose a reason for hiding this comment

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

The most RESTful thing would've been to put the breed as a query param (/cats?breed=bengal) instead of creating its own route. This way, you've still named the endpoint after what it returns: cats (filtered on breed)

Copy link
Author

Choose a reason for hiding this comment

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

I thought about that first but then it said in the requirements that we should have "A minimum of one endpoint to return a single result (single element)." so then I added it as a route parameter so I had 2 of the single element endpoints.

// Combined cats query parameter filtering on all cats and cats with category query filter for personality, fur length and commonality.

app.get("/cats", (request, response) => { // http://localhost:9000/cats -route for displaying all cat breeds.
const {personality, fur_length, commonality}= request.query
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Here you could've added breed as well

Comment on lines +81 to +85
if (cat) {
response.status(200).json(cat)
} else {
response.status(404).send("No cat found with that ID")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation!

app.get ("/cats/breed/:breed", (request, response) => {
const breed = request.params.breed.trim().toLowerCase()

const cat = cats.find (cat => cat.breed.toLowerCase() === breed) //Find specific cat breed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you think of a better variable name? 👀

Copy link
Author

Choose a reason for hiding this comment

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

I thought breed was a good name since I only have one key in the json file called breed. But another suggestion could be requestedBreed.

const cat = cats.find (cat => cat.breed.toLowerCase() === breed) //Find specific cat breed.
//https://project-express-api-cats.onrender.com/cats/breed/russian%20blue
if (cat) {
response.status (200).json (cat)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice that you included status codes! But both status and json are JS methods and therefor it should not be a space between method and ()

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.

2 participants