Ari - Scissors Class ✂️ #50
Conversation
There was a problem hiding this comment.
Good work working through some tricky logic for this program. You've made good use of instance methods to encapsulate functionality. I've left inline comments on ways to consider refactoring. One place I would encourage you to focus with the next project is being consistent with how code is written to increase readability. Removing comments can also increase readability. I know this sort of refactoring is tricky when there's time pressure. Please let me know if you have any questions.
| if not video or not customer: | ||
| return {"details": "Not Found"}, 404 | ||
| # need to check if video id is in rental | ||
| elif not "video_id" in request_body or \ | ||
| not "customer_id" in request_body: | ||
| return {"details": "Bad Request"}, 400 | ||
| elif not rental: | ||
| return {"details": "Bad Request"}, 400 | ||
| elif customer.videos_checked_out_count < 1: | ||
| return {"details": "Bad Request"}, 400 |
There was a problem hiding this comment.
Consider providing more details about why the request is bad, such as "That customer doesn't have any Videos checked out", "There is no rental with that customer_id and video_id", "You must provide a customer_id and video_id to look up the rental"
| return {"details": "Bad Request"}, 400 | ||
| else: | ||
| # once all fields valid, will create rental and update due date | ||
| new_rental = Rental.from_json_to_check_out(request_body) |
There was a problem hiding this comment.
Hrmm.. We do need to create a new rental when we check a video in. Instead, we might decide to update the status of the rental to "checked-in" or delete the rental instance entirely.
| return {"details": "Bad Request"}, 400 | ||
| else: | ||
| # once all fields valid, will create rental and update due date | ||
| new_rental = Rental.from_json_to_check_out(request_body) |
There was a problem hiding this comment.
Great work encapsulating functionality in a helper method from_json_to_check_out. YOu could consider putting additional logic into this method such as the logic below. You would need to pass these instances to the method.
customer.videos_checked_out_count += 1
video.available_inventory -= 1
| def retrieve_videos_checked_out_by_customer(customer_id): | ||
| customer = Customer.query.get(customer_id) # querying customer by given id | ||
| if customer == None: | ||
| return jsonify('Not Found'), 404 |
There was a problem hiding this comment.
Above you use this code return {"details": "Not Found"}, 404 instead. One isn't better than the other, but being consistent can enhance readability.
| "due_date": rental.due_date.strftime("%a, %d %b %Y %X %z %Z")} | ||
|
|
||
| @staticmethod | ||
| def customer_due_date_response(rental, customer): |
There was a problem hiding this comment.
Since this is an instance method, we do not need to pass in the instance of rental, we can use self
| registered_at = db.Column(db.DateTime, \ | ||
| default=datetime.now(), | ||
| nullable=False) | ||
| videos_checked_out_count = db.Column(db.Integer, default=0) |
There was a problem hiding this comment.
Instead of videos_checked_out_count being a column we can use the rentals to count that.
To do that it's helpful to add a relationship:
rentals = db.relationship('Rental', back_populates='customer', lazy=True)
For to_dict, once you have the relationship the videos_checked_out_count is the length of the rentals:
videos_checked_out_count": len(self.rentals)
This code wouldn't work exactly as written with your implementation since a rental is created both at check-in and check-out. The code written here assumes a rental is deleted when it was checked it. We might also consider changing the status of the rental to "checked-in" and then writing logic just to find rentals that are checked out.
This method of dynamically computing videos_checkout_out_count rather than storing it in an instance variable makes it so there isn’t information stored in multiple places that could potentially conflict.
| title = db.Column(db.String) | ||
| release_date = db.Column(db.DateTime, nullable=False) | ||
| total_inventory = db.Column(db.Integer, nullable=False) | ||
| available_inventory = db.Column(db.Integer, nullable=False) |
There was a problem hiding this comment.
Like with videos_checked_out_count, consider how you might calculate this value rather than storing it as an attribute in the database.
No description provided.