Ocelots - Eva and Lisa #16
Conversation
Eva wave03
| @@ -0,0 +1,153 @@ | |||
| from app import db | |||
There was a problem hiding this comment.
If there's a single route file it's okay to leave it in the app folder, but if we have more than one, for organization we should create a folder to hold all the route files.
| videos_checked_out_count = db.Column(db.Integer, default=0) | ||
| videos = db.relationship("Video", secondary="rental", back_populates="customers") |
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 list of the videos the user currently has checked out and videos_checked_out_count. We could remove videos_checked_out_count and call len on videos anywhere we need the count.
| postal_code = db.Column(db.String) | ||
| phone = db.Column(db.String) |
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
| @classmethod | ||
| def from_dict(cls, data): | ||
| pass |
There was a problem hiding this comment.
This is okay for personal projects as reminders, but when working in coding teams I would suggest removing function stubs like this before opening PRs.
| customer_id = db.Column(db.Integer, db.ForeignKey('customer.id'), primary_key=True, nullable=False) |
There was a problem hiding this comment.
Really nice use of a composite primary key so we don't need to store a separate id attribute!
| customer_rental_response = [] | ||
| for rental in rentals: | ||
| customer_rental_response.append(rental.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?
| customer = validate_model(Customer, check_in_data["customer_id"]) | ||
| video = validate_model(Video, check_in_data["video_id"]) | ||
| except KeyError as err: | ||
| abort(make_response({"message":f"Missing {err.args[0]}."}, 400)) |
There was a problem hiding this comment.
validate_model will call abort with a message. For my knowledge, why do we want to catch the error and call abort here?
| rentals = Rental.query.all() | ||
| rental_found = False | ||
| rental_count = 0 | ||
|
|
||
| for rental in rentals: | ||
| if rental.video_id == video.id: | ||
| rental_count += 1 | ||
| if (rental.customer_id == customer.id | ||
| and rental.video_id == video.id): | ||
| rental_found = True | ||
|
|
||
| for rental in rentals: | ||
| if rental.video_id == video.id and rental.customer_id == customer.id: | ||
| rental_found = True |
There was a problem hiding this comment.
We could handle this many ways. If we are using a query to grab the rentals, then I would recommend using SQLAlchemy's tools to manage the filtering. You could do something like:
Rental.query.filter_by(customer_id=customer_id, video_id=video_id).first()Another option: Since we look up a reference to the customer on line 68, and it has an attribute video that holds their current rentals, we could filtering that list by the video's id to see if there's an existing rental.
| for rental in rentals: | ||
| if rental.video_id == video.id and rental.customer_id == customer.id: | ||
| rental_found = True |
There was a problem hiding this comment.
This loop duplicates what we're doing in the if-statement above, I think it could be removed.
|
|
||
| # check if the customer did rent the video | ||
| rentals = Rental.query.all() | ||
| rental_found = False |
There was a problem hiding this comment.
Instead of a boolean, I suggest creating a variable initialized to None that will be assigned to the rental we are looking for on line 91 if we find it. By holding onto a reference of the rental we want to delete, we can can call db.session.delete() on it and avoid making another database query down on line 117.
No description provided.