Ac2-Ocelots-Catherine-Bandarchuk(Co-worker:Yvette Wu):retro-vide-store-api #9
Conversation
Kate retro video store
| phone = db.Column(db.String, nullable = False) | ||
| videos_checked_out_count = db.Column(db.Integer, default=0) | ||
| videos_checked_in_count = db.Column(db.Integer, default=0) | ||
| videos = db.relationship("Rental") |
There was a problem hiding this comment.
If we want a bidirectional relationship where the videos attribute on this model is updated when the connected Rental attribute customers is updated, we should specify back_populates="customers" here. I'd have to do some digging into why this is working with back_populates only specified on one side, but it is against best practices and could lead to undefined or confusing behavior.
There was a problem hiding this comment.
If I understand the question correct we have costomer-video relationship in rental_model.py:
customers = db.relationship("Customer", back_populates="videos")
videos = db.relationship("Video", back_populates="customers") so we actually create bidirectional relationship between these two models.
In customer and video models we have relationship with rental showing that we'll receive customers of video through Rental model. The same with with video - (Rental) - customer relationship.
It's my understanding, maybe Yvette can explain it better.
| videos_checked_out_count = db.Column(db.Integer, default=0) | ||
| videos_checked_in_count = db.Column(db.Integer, default=0) |
There was a problem hiding this comment.
In most situations, if we can derive the data we want from something else we are already storing, we should choose to do that over storing a new piece of data. If we store a piece of derivable data, we need to manually keep our related data in sync which introduces places for potential bugs, and we take up more memory for each record.
In this case, we hold videos which is a history of our customer's rentals. We could remove videos_checked_out_count and videos_checked_in_count by filtering the videos list by their status attribute and calling len on the resulting list anywhere we need one of the counts.
There was a problem hiding this comment.
Totally agree. We added status attribute on the wave 4 and basically didn't have time to refactored and make the program more "dry" and effective of using memory.
| postal_code = db.Column(db.String, nullable = False) | ||
| phone = db.Column(db.String, nullable = False) |
There was a problem hiding this comment.
It isn't necessary for this project, but out in the wild we'd likely look at what the max size these strings could possibly be is and give character limits in the definition. The top answers here give some explanation of possible performance impacts: https://stackoverflow.com/questions/1962310/importance-of-varchar-length-in-mysql-table
| id = db.Column(db.Integer, primary_key=True, autoincrement=True, nullable=False) | ||
| due_date = db.Column(db.DateTime, default=datetime.now()+timedelta(days=7), nullable=False) | ||
| status = db.Column(db.String, default="Checked out", nullable=False) |
There was a problem hiding this comment.
not gonna take credit for it, i learned from (Eva, Lisa group) and Cindy
| #__tablename__ = 'rental_table' | ||
| id = db.Column(db.Integer, primary_key=True, autoincrement=True, nullable=False) |
There was a problem hiding this comment.
Another option could be to create a composite primary key from the customer_id and video_id columns. The benefit is we can store one less column for each record, but the limitation is there can only ever be one Rental record with that combination of customer and video ids.
# One possible set up
video_id = db.Column(db.Integer, db.ForeignKey('video.id'), primary_key=True, nullable=False)
customer_id = db.Column(db.Integer, db.ForeignKey('customer.id'), primary_key=True, nullable=False)| db.session.refresh(new_rental) | ||
| db.session.refresh(video) | ||
| db.session.refresh(customer) |
There was a problem hiding this comment.
Of these 3 refresh calls, I believe we only need to refresh new_rental to ensure the ID is assigned. The other 2 we have updated the references we read for the response dict directly on lines 41 and 42, so they should be up to date.
| if check_outstanding: | ||
| abort(make_response(jsonify(check_outstanding), 400)) | ||
|
|
||
| return_rental = Rental.query.filter_by(customer_id=customer_id, video_id=video_id, status="Checked out").order_by(Rental.due_date.asc()).first() |
There was a problem hiding this comment.
We should use suggestions from PEP8 to drop this over a couple lines to stay under the best practice of 79 characters max per line. https://peps.python.org/pep-0008/#maximum-line-length
I see several lines across the project this applies to, so just something to keep an eye out for in the future.
| if check_outstanding: | ||
| abort(make_response(jsonify(check_outstanding), 400)) | ||
|
|
||
| return_rental = Rental.query.filter_by(customer_id=customer_id, video_id=video_id, status="Checked out").order_by(Rental.due_date.asc()).first() |
There was a problem hiding this comment.
This is a solid approach, another one would be to filter the videos attribute of the customer we looked up on line 68 for the video_id we're looking for.
| customers_list = [] | ||
| for customer in customers: | ||
| customers_list.append(customer.to_dict()) |
There was a problem hiding this comment.
This would be a great candidate for a list comprehension - what could that look like? Are there other places you see across the project where a list comprehension would make sense?
There was a problem hiding this comment.
customer_list = [customer.to_dict() for customer in customers]
| db.session.refresh(video) | ||
|
|
||
| return video.to_dict() | ||
|
|
There was a problem hiding this comment.
Small style note: since the other files use 2 blank lines to separate functions in a file, we should be consistent with that across this file as well.
No description provided.