Spruce - Gabe and Asha #40
Conversation
There was a problem hiding this comment.
Well done!
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. For example, the logic around checking whether an id is compatible with the get method could reside in a custom method to retrieve model records more safely. Or the rental queries could be moved into helpers on customer or video (or a neutral 3rd part class) to avoid needing to remember all the details of the query each time it needs to be written.
We can also think about how to simplify our routes (and get closer to the single responsibility principle) by reducing the amount of logic directly written in them. 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. 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.
Overall, nicely done! 🎉
| @@ -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.
| from .routes import customers_bp, videos_bp, rentals_bp | ||
| app.register_blueprint(customers_bp) | ||
| app.register_blueprint(videos_bp) | ||
| app.register_blueprint(rentals_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)| registered_at = db.Column(db.DateTime) | ||
| postal_code = db.Column(db.String) | ||
| phone = db.Column(db.String) |
There was a problem hiding this comment.
Keep in mind that the default value for nullable is True, so if we don't set it to false, the columns will be allowed to be NULL. 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 (which is a good thing)!
| video_id = db.Column(db.Integer, db.ForeignKey('video.id'), primary_key=True, nullable=False) |
There was a problem hiding this comment.
I generally agree with your decision here to make the foreign keys not nullable. A rental without a customer or video is of questionable value. However, when a row referenced by a foreign key is deleted, postgres attempts to resolve the lost data by setting those foreign keys to NULL, which this constraint prevents, and would cause the final tests (deleting videos and customers in a rental) to fail. In this case, the secondary relationship is what is allowing those tests to pass, as it causes SQLAlchemy to delete the rows in the secondary table that depend on a deleted foreign key. This may be what we want, but on the other hand, data is very valuable to a company. Just be sure you have considered the alternatives and are intentionally choosing an approach after weighing your options.
| postal_code = db.Column(db.String) | ||
| phone = db.Column(db.String) | ||
| videos = db.relationship("Video", secondary="rental", backref="customer") |
There was a problem hiding this comment.
Notice that your code doesn't use this relationship in the logic anywhere. Using the secondary attribute on a relationship is more appropriate when we have a simple many-to-many relationship, such as with a pure join table. In that case, a relationship like this would allow SQLAlchemy to automatically manage the join table between the two models being linked and we could establish those links by using the relationship like a collection, by appending, removing, etc. In that case, the backref attribute supplies the equivalent name to attach to the model on the other end of the relationship. Since the local name here is videos, a closer match in semantics for the other side might be customers rather than customer, since there are potentially many customers who have rented a video (though again, this isn't used in your code).
However, in this application, our rental relationship between videos and customers isn't a pure join table. It has its own information that also needs to be tracked, and which SQLAlchemy can't automatically manage (such as the checked in status, and due date). In this case, using reciprocal one-to-many relationships can bring us some of the benefits of moving around the relationship links, while still leaving it to us to explicitly set up the rental model ourselves. Consider a configuration like the following:
rentals = db.relationship("Rental", backref="customer")If we had a customer model in the variable customer, then we could access the rentals associated with the customer as customer.rentals. If there were a similar relationship on the video side, then to get the video title of a particular customer rental, we could write code like customer.rentals[0].video.title. SQLAlchemy takes care of using the foreign key information to fill in the relationships.
| customer = Customer.query.get(customer_id) | ||
| video = Video.query.get(video_id) |
There was a problem hiding this comment.
Even here, we should ensure that the ids are of the proper format. With the number of places this is required, a helper class method in the model types would be helpful.
| num_of_videos_rented = Rental.query.filter_by( | ||
| video_id=video.id, checked_out=True).count() | ||
| available_inventory = video.total_inventory - num_of_videos_rented |
There was a problem hiding this comment.
Consider moving the logic for calculating the checked out count and available inventory to helper methods in the Rental model.
| rental = Rental.query.filter_by( | ||
| video_id=video.id, customer_id=customer.id, checked_out=True).first() |
There was a problem hiding this comment.
Consider ordering the rentals by the due date. A customer could have multiple rentals for the same video outstanding, and it would be nice to guarantee the oldest rental is considered to be returned first.
| num_of_videos_rented = Rental.query.filter_by( | ||
| video_id=video.id, checked_out=True).count() |
There was a problem hiding this comment.
I do like your use of count() when you just need the number of records. This saves the database from having to send back the full data of the rows, and the database itself is very good at counting rows.
| rentals = Rental.query.filter_by( | ||
| customer_id=customer.id, checked_out=True).all() |
There was a problem hiding this comment.
You've done a good job of remembering to add the checked_out=True in your queries, but notice how easy it would be to forget, or if there was a new dev on your team, it might not occur to them they need it. Moving even a single complex line into a well-named helper function can be very useful for avoiding errors.
No description provided.