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

Nodejs week3 #3

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Nodejs week3 #3

wants to merge 7 commits into from

Conversation

Rajesh-Bhatt
Copy link
Owner

@Rajesh-Bhatt Rajesh-Bhatt commented Oct 3, 2024

Meal sharing endpoints implemented:

Added more parameters to the meals route.
Created the set of routes for reviews as well.

Copy link

@kralle333 kralle333 left a comment

Choose a reason for hiding this comment

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

Great work!

I can see you worked hard on this and the comments I have will just make the solution even more solid or just improve the responses that the user is receiving :)

Keep it up!

Comment on lines +26 to +39
query
.leftJoin("reservation", "meal.id", "=", "reservation.meal_id")
.select("meal.id", "meal.max_reservations", "meal.title")
.sum("reservation.number_of_guests as sum_of_guests")
.groupBy("meal.id", "meal.max_reservations", "meal.title")
.having("sum_of_guests", "<", knex.ref("meal.max_reservations"));
} else {
query
.leftJoin("reservation", "meal.id", "=", "reservation.meal_id")
.select("meal.id", "meal.max_reservations", "meal.title")
.sum("reservation.number_of_guests as sum_of_guests")
.groupBy("meal.id", "meal.max_reservations", "meal.title")
.having("sum_of_guests", ">=", knex.ref("meal.max_reservations"));
}

Choose a reason for hiding this comment

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

these two queries are almost the same. You can merge the two by using ternary conditional operator in the having condition?ifTrue:ifFalse

Comment on lines +41 to +52
if (title !== undefined) {
query.where("title", "like", `%${title}%`); //performing a partial match here
}
if (dateAfter !== undefined) {
query.where("when", ">", dateAfter);
}
if (dateBefore !== undefined) {
query.where("when", "<", dateBefore);
}
if (limit !== undefined) {
query.limit(limit); //Returns the given number of meals
}

Choose a reason for hiding this comment

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

you are checking if the values in the request are undefined, but what if they are not, but incorrect data types? Validate the values and then return proper responses to the user so they know how to formulate the request correctly :)

Comment on lines +136 to +148
mealsRouter.get("/:meal_id/reviews", async (req, res, next) => {
try {
const id = req.params.meal_id;
const reviewsForMeal = await knex("review").where("meal_id", id);
if (reviewsForMeal.length == 0) {
res.json({ message: "Meal not found" });
} else {
res.json(reviewsForMeal);
}
} catch (error) {
next(error);
}
});

Choose a reason for hiding this comment

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

Almost there :) please check https://stackoverflow.com/questions/13366730/proper-rest-response-for-empty-table for how to handle this scenario.

reservationsRouter.delete("/:id", async (req, res) => {
try {
const id = req.params.id;
await knex("reservation").where({ id: id }).del();

Choose a reason for hiding this comment

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

what if the reservation with this id doesnt exist? I guess it would be good to tell the user :)

Comment on lines +7 to +19
reviewRouter.get("/", async (req, res, next) => {
try {
const allReviews = await knex("review");

if (allReviews.length === 0) {
return res.status(404).json({ message: "No reviews found" });
}

res.json(allReviews);
} catch (error) {
next(error);
}
});

Choose a reason for hiding this comment

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

});

// Updates the review by id.
reviewRouter.put("/:id", async (req, res, next) => {

Choose a reason for hiding this comment

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

Good job

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