Retro-Video-Store - Spruce Veronica & Angela #54
Conversation
There was a problem hiding this comment.
Great job!
All your tests are passing and your code is very readable.
Try looking for repetitive parts of the code and moving them to helper methods. Also, try to make your functions follow the single responsibility principle. So for example, think about splitting your route endpoint functions so that each function handles only one verb.
This also extends to the level of abstraction that we deal with in our routes. The main job of a route should be to get the data from the request, then call helper methods to do the real work (business logic), and finally take the results and package them to be sent back to the client. Currently, there's business logic (such as how to update the database to reflect checking out and in) in your routes. Think about moving that to methods in your existing model classes, or even creating entirely new classes that represent sequences of actions carried out at the level of business logic. Consider adding a class like RentalManager, that could have check_out and check_in methods to manage those workflows.
In the database, pay attention to what choosing to allow or disallow nulls implies for your data relationships. What would it mean if each value that's allowed to be null actually was null, especially for foreign key relationships? Sometimes, we do want data to be nullable, essentially to make it optional. But if the data is required, we should use the database to help us enforce that.
But overall, nicely done! 🎉
| from .routes import rentals_bp, videos_bp, customers_bp | ||
| app.register_blueprint(rentals_bp) | ||
| app.register_blueprint(videos_bp) | ||
| app.register_blueprint(customers_bp) |
There was a problem hiding this comment.
👍
One thing we can do to help our routes file from getting too large is to consider making multiple files containing routes separated by the resource type. We might have a routes folder instead of a routes.py file, and inside that folder (along with a __init__.py) we could have a file per resource, so customer.py, video.py, and rental.py. Where each would have a blueprint and endpoints for that resource. When we have one blueprint per file, we often name the blueprint simply bp rather than including the resource name as part of it.
Then here, we could import and register the blueprints like
from .routes import customer, video, rental
app.register_blueprint(customer.bp)
app.register_blueprint(video.bp)
app.register_blueprint(rental.bp)| postal_code = db.Column(db.String) | ||
| phone = db.Column(db.String) | ||
| registered_at = db.Column(db.DateTime, nullable=True) |
There was a problem hiding this comment.
Keep in mind that the default value for nullable is True. When adding column definitions to our models, we should consider whether it makes sense to allow a record to exist without that data, such as a Customer without a name, a Video without a title, or a Rental without a due date. It's true that we can add checks for this in our logic that adds entries, but we should try to leverage as much validation help from the database as possible. If we tell the database that certain columns are required (not nullable), then we'll encounter an error if we accidentally try to create a row without a value for those columns!
| phone = db.Column(db.String) | ||
| registered_at = db.Column(db.DateTime, nullable=True) | ||
| rentals = db.relationship('Rental', backref='customer', lazy=True) |
There was a problem hiding this comment.
👍
Nice job establishing this relationship from the customer to the rental through the one to many foreign key relationship. Notice that this record doesn't actually change the schema in any way, but makes use of the foreign key information between the two to add this attribute to the class (and a customer attribute to the rental class via the backref) to make working with the relationship between the two a little easier.
| rentals = db.relationship('Rental', backref='customer', lazy=True) | ||
|
|
||
| def customer_dict(self): |
There was a problem hiding this comment.
👍
Nice method to convert the model into a dict for returning. We have a variety of endpoint outputs that we need to handle in this project, but having a general-purpose dicttionary converter is a great start!
| video_id = db.Column(db.Integer, db.ForeignKey('video.id')) |
There was a problem hiding this comment.
Like any other column, the default nullable value for foreign keys is True. Allowing the foreign keys to be nullable has some benefits, as well as some drawbacks. On the plus side, it makes it easier to delete a customer or video that is referenced in a rental (checked in OR checked out). When we try to delete a row, Postgres will try to nullify any foreign key column that references the primary key of the row being deleted. Since the columns are set to allow nulls, this works fine and the row (customer or video) can be deleted. However, the rental row is now left with some of its data missing. The rental may have a customer, but no video, or vice versa. Do we want our data to look like this? Maybe, but we would want to be sure that it was no longer counted as an outstanding rental against the remaining customer or video.
On the other hand, if we don't allow the columns to be null, then the attempt by postgres to nullify the column will violate the constraints, and will fail. In that case, we would need to look into how to get postgres to do more than just nullify the columns, we would need it to delete the dependent rows themselves (or do it manually, or take a different approach).
Either approach (and others) can work, so long as we deal with the effects.
| if customer is None: | ||
| return jsonify ({"message": "Customer does not exist"}), 404 | ||
|
|
||
| videos_checked_out = Rental.query.filter(Rental.video_id == request_body["video_id"], Rental.checked_out == True).count() |
There was a problem hiding this comment.
Getting the checked out count for a video could be a useful operation to use in other situations (even just a few lines later!). Consider moving this logic into a helper method on video.
|
|
||
| videos_checked_out = Rental.query.filter(Rental.video_id==request_body["video_id"], Rental.checked_out==True).count() | ||
|
|
||
| available_inventory = video.total_inventory - videos_checked_out |
There was a problem hiding this comment.
The available inventory calculation could also be moved into the video class as a helper method.
| new_rental = Rental( | ||
| video_id=request_body["video_id"], | ||
| customer_id=request_body["customer_id"], | ||
| checked_out=True | ||
| ) |
There was a problem hiding this comment.
We should set the due date as part of creating the rental.
| if video is None or customer is None: | ||
| return jsonify({"message": "Video and customer does not exist"}), 404 | ||
|
|
||
| rental = Rental.query.filter(Rental.video_id == request_body["video_id"], Rental.customer_id == customer.id, Rental.checked_out == True).first() |
There was a problem hiding this comment.
Consider moving this logic into a static/class method of rental. Essentially, a way to find a rental based on a video and customer id. Also, consider sorting the rentals such that the oldest rental record is found. A customer could have multiple copies of the same video checked out, and it would be nice if we could guarantee that they are checked back in from oldest to newest.
| @@ -0,0 +1,28 @@ | |||
| """empty message | |||
There was a problem hiding this comment.
Remember to add a message to your migrations with -m when running the migration step.
No description provided.