Ocelots - Diana Arreola, Supriya Chavan, Yuan Du #17
Conversation
| 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
| videos_checked_out_count = db.Column(db.Integer, default=0) | ||
| rentals = db.relationship("Rental", back_populates="customer") |
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 rentals which is a history of our customer's rentals. We could remove videos_checked_out_count by filtering the rentals list by their check_out_status attribute and calling len on the resulting list anywhere we need the counts.
There was a problem hiding this comment.
We added the check_out_status at the end and didn't realize or think that we could refactor customer model and routes . But it's a good learning to check all the models and endpoints if we can derive some attributes based on the change.
| id = db.Column(db.Integer, primary_key=True,nullable=False) | ||
| due_date = db.Column(db.DateTime, default=(datetime.date.today()+datetime.timedelta(days=7))) | ||
| check_out_status = db.Column(db.Boolean,default = True) |
| except KeyError as key_error: | ||
| abort(make_response({"message":f"Could not perform checkout bc {key_error.args[0]}"}, 400)) | ||
|
|
||
| db.session.add(rental) |
There was a problem hiding this comment.
Since we are not adding a new rental, just updating an existing one, we should be able to remove this add call.
| id = db.Column(db.Integer, primary_key=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)There was a problem hiding this comment.
We had the idea that one customer can rent multiple copies of the same video so but we will definitely look into the composite primary key for any other applications we might use.
| def create_rental(): | ||
| request_body = request.get_json() | ||
|
|
||
| try: |
There was a problem hiding this comment.
This is a lot of code to surround in a try/catch and is causing the main flow of our code to be indented when most of it doesn't need to be. What are the relevant parts that could throw the error? I would surround just those lines so it's clear what the try/catch is targeting.
Another possibility could be to do away with the try/catch entirely and use the get function passing a default argument so that if the key doesn't exist, it returns a default like None that would fail in validate_model and send the user a message when we hit an abort call.
customer_id = dict.get("customer_id", default=None)
customer = validate_model(Customer, customer_id)There was a problem hiding this comment.
This is smart way of using our existing abort call code block. Thank You.
| rental = db.session.query(Rental).filter_by(video_id=video.id, customer_id=customer.id).first() # if customer has more than two rentals | ||
| # checked_in_rental = validate_model(Rental, {"id": rental.id, "customer_id": rental.customer_id, "video_id": rental.video_id}) |
There was a problem hiding this comment.
Since we have a reference to the customer on line 56, we could also look at filtering its rentals attribute by video id.
matching_rentals = [rental for rental in customer.rentals
if rental.video_id == video.id]There was a problem hiding this comment.
Thank you Kelsey! This makes a ton of sense, I don't think initially I understood the relationships we created in our models, and now it's beginning to make a lot more sense
| return model | ||
|
|
||
| # return True if its a valid number, return False it's not a number | ||
| def validate_num_queries(query_param): |
There was a problem hiding this comment.
Since this function is not shared between multiple route files, I would consider placing it in the specific route file that uses it.
| db.session.commit() | ||
|
|
||
| video_response = new_video.to_dict() | ||
| return jsonify(video_response),201 |
There was a problem hiding this comment.
Gentle reminder that we want a space on the right side of commas to have a visual separation between arguments.
|
|
||
|
|
There was a problem hiding this comment.
Inside of a function it is against best practices to use double blank lines. White space like this is generally used to give readers a visual cue that the code above and below does very different things, often separating blocks of functions or code that are related but have a different use. An example would be if we had routes and helper functions in this file - it would be common to use 2 blank lines to separate the block of route functions from the block of helper functions.
|
Thank you for the thorough comments! @kelsey-steven-ada |
No description provided.