Spruce - C16 - Symone & Vange #51
Conversation
There was a problem hiding this comment.
Great work Symone and Vange! Y'all met all the learning goals for this project and passed all the tests. Y'all did a great job practicing the single responsibility principle when it came to defining routes.
I provided feedback on how y'all can refactor by omitting redundant logic and reorganizing your project.
Project grade: Green 🟢 !
| @@ -32,5 +32,10 @@ def create_app(test_config=None): | |||
| migrate.init_app(app, db) | |||
|
|
|||
| #Register Blueprints Here | |||
|
|
|||
| from .routes import customers_bp, videos_bp, rentals_bp | |||
| name = db.Column(db.String) | ||
| postal_code = db.Column(db.String) | ||
| phone = db.Column(db.String) | ||
| registered_at = db.Column(db.DateTime()) | ||
| videos = db.relationship("Video", secondary="rental", backref="customers") |
There was a problem hiding this comment.
At the moment, any of these columns can contain null values. Consider revisiting each column to determine which column should truly have null values versus not. For example, is it useful to store a Customer without a name or phone number? Those two columns can be considered required information to store about a customer, despite our requirements not strictly describing that behavior.
| id = db.Column(db.Integer, primary_key=True, autoincrement=True) | ||
| title = db.Column(db.String) | ||
| release_date = db.Column(db.DateTime()) | ||
| total_inventory = db.Column(db.Integer) | ||
|
|
||
| def to_dict(self): | ||
| return { | ||
| "id": self.id, | ||
| "title" : self.title, | ||
| "release_date" : self.release_date, | ||
| "total_inventory" : self.total_inventory | ||
| } | ||
|
|
||
| @classmethod | ||
| def from_dict(cls, values): | ||
| return cls(**values) |
There was a problem hiding this comment.
The same comment about required information for Customer can also be applied to Video and Rental.
| @@ -0,0 +1,413 @@ | |||
| from app import db | |||
There was a problem hiding this comment.
This file is over 400 lines of code! We can refactor the overall project structure by adding the logic specific to the customer, video, and rental routes into separate files within a routes folder.
| def display_customer_info(customer): | ||
| return { | ||
| "id": customer.id, | ||
| "name": customer.name, | ||
| "postal_code": customer.postal_code, | ||
| "phone": customer.phone | ||
| } |
There was a problem hiding this comment.
Nice helper function. We could move this function into the Customer model and that way every instance of a Customer will have access to this method, eliminating the need to pass in the instance as a parameter.
| @videos_bp.route("/<video_id>", methods = ["DELETE"]) | ||
| def delete_video(video_id): |
| if "customer_id" not in request_body: | ||
| return {"details": "Request body must include customer_id."}, 400 | ||
|
|
||
| if "video_id" not in request_body: | ||
| return {"details": "Request body must include video_id."}, 400 |
There was a problem hiding this comment.
We can combine these conditionals
| if "customer_id" not in request_body: | |
| return {"details": "Request body must include customer_id."}, 400 | |
| if "video_id" not in request_body: | |
| return {"details": "Request body must include video_id."}, 400 | |
| if "customer_id" not in request_body or "video_id" not in request_body: | |
| return {"details": "Request body must include customer_id."}, 400 |
| return jsonify(response_body), 200 | ||
|
|
||
|
|
||
| @rentals_bp.route("/check-in", methods = ["POST"]) |
| @customers_bp.route("/<customer_id>/rentals", methods=["GET"]) | ||
| def get_checked_out_videos(customer_id): |
| @videos_bp.route("/<video_id>/rentals", methods=["GET"]) | ||
| def get_customers_by_video(video_id): |
No description provided.