Nodejs/week3/stefany #246
Nodejs/week3/stefany #246
Conversation
LucasIversen
left a comment
LucasIversen
left a comment
There was a problem hiding this comment.
Hi Stefany. I've got a few comments, mainly regarding how we fetch data. If anything needs further explaining, you can always write me on slack 👌
| try { | ||
| let meals; | ||
| if (!req.query.availableReservations) { | ||
| meals = await getAllMeals(); |
There was a problem hiding this comment.
I see what you're doing. It's sometimes easier to filter on an array in javascript than expanding the knex query. However, this also means that we always fetch every single meal, even when we maybe only need to return a couple of them. Now imagine we have 10000 meals, that's a lot of data to fetch every time. Therefore, it's better practice to make the query only fetch what you need.
| }; | ||
|
|
||
| //sortDir | ||
| if (req.query.sortDir) { |
There was a problem hiding this comment.
The sortDir should only be used if the sortKey is also used.
| try { | ||
| const mealId = +req.params.id; | ||
| const reqBody = req.body; | ||
| const meal = await knex("meals").where({ id: mealId }).first(); |
There was a problem hiding this comment.
We can avoid this fetch by checking the updatedMealNum instead
| //get/reviews/:id | ||
| const getReviewById = async (id) => { | ||
| try { | ||
| const allReviews = await getAllReviews(); |
There was a problem hiding this comment.
Here we also fetch all the reviews only to return a single one. Let's instead only fetch the meal with the id instead
No description provided.