Sierra and Melinda - Pine #60
Conversation
There was a problem hiding this comment.
Fantastic work on this project Sierra and Melinda! I like the way you are generating the available inventory dynamically by pulling data from the rental table. My recommendation would be to think about ways to create helper functions to break up some of the longer functions and pull out some of the chunks of similar code. The functionality on this project is very solid and you hit all of the learning goals. This project is green!
|
|
||
| def rental_dict(self, id): | ||
| video = Video.query.get(id) |
There was a problem hiding this comment.
The db.relationships is already doing this work for you. It looks like the relationships are set up correctly so self.video should be a Video model where the Video.id == self.video_id.
| customers_response = [] | ||
|
|
||
| for customer in customers: | ||
| customers_response.append(customer.to_dict()) |
There was a problem hiding this comment.
This works well and is very readable, but if you'd like to start incorporating list comprehensions into your code, a loop like this that builds a list is a great place to start:
| customers_response = [] | |
| for customer in customers: | |
| customers_response.append(customer.to_dict()) | |
| customers_response = [customer.to_dict() for customer in customers] |
| if customer is None: | ||
| return make_response({"message": f"Customer {customer_id} was not found"}, 404) | ||
| elif "name" not in request_body or "postal_code" not in request_body or "phone" not in request_body: | ||
| return make_response({"details": "Invalid data"}, 400) | ||
| else: | ||
| response_body = request.get_json() |
| @videos_bp.route("", methods=["GET"]) | ||
| def get_all_videos(): | ||
| videos = Video.query.all() | ||
| response_body = [video.to_dict() for video in videos] | ||
| return jsonify(response_body), 200 |
There was a problem hiding this comment.
This isn't getting called. This is the same route as line 92 and I suspect what is happening here is that the first route is the one that is called and this one is ignored.
| if video is None: | ||
| return make_response({"message": f"Video {video_id} was not found"}, 404) |
There was a problem hiding this comment.
This block is the same for all methods, I recommend bumping this up out of the if/elif structure to right after line 136 to DRY the code slightly.
| if video.total_inventory - len(video.rentals) == 0: | ||
| abort(make_response({"message": "Could not perform checkout"}, 400)) |
There was a problem hiding this comment.
There is a small logical bug here. The list of rentals associated with a video will include all of the rentals where that have a foreign key video_id that matches the id of this video. The check-in logic sets the checked-out variable of the rental to False, but does not delete the rental. This list will then contain all of the current and past rentals. One way to fix this is to go through the list of rentals and count the ones that are active:
| if video.total_inventory - len(video.rentals) == 0: | |
| abort(make_response({"message": "Could not perform checkout"}, 400)) | |
| active_rentals = 0 | |
| for rental in video.rentals: | |
| if rental.checked_out: | |
| active_rentals += 1 | |
| if video.total_inventory - active_rentals == 0: | |
| abort(make_response({"message": "Could not perform checkout"}, 400)) |
| num_currently_checked_out = Rental.query.filter_by(video_id=video.id, checked_out=True).count() | ||
| available_inventory = video.total_inventory - num_currently_checked_out | ||
| videos_customer_checked_out = Rental.query.filter_by(customer_id=customer.id, checked_out=True).count() |
There was a problem hiding this comment.
💯 The query on line 244 could also be used to fix the logical bug in the check out method.
| elif "name" not in request_body or "postal_code" not in request_body or "phone" not in request_body: | ||
| return make_response({"details": "Invalid data"}, 400) |
There was a problem hiding this comment.
This section is very similar to the validation in the POST route in the handle_customers() function. I recommend thinking about ways to pull these sections out into helper functions to DRY the code.
No description provided.