Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maple-AndreaY&Elly.-C16 #50

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Maple-AndreaY&Elly.-C16 #50

wants to merge 4 commits into from

Conversation

ASY13
Copy link

@ASY13 ASY13 commented Nov 16, 2021

No description provided.

Comment on lines +23 to +27
# pass

# def append_to_list(self, customers_json):
# return [customer for customer in customers_json]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove unused code

delete-orphan", lazy="joined")

def to_json(self):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great instance method

Comment on lines +13 to +14
# available_inventory = db.Column(db.Integer)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove unused code

@@ -0,0 +1,9 @@
# from flask.json import jsonify
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you plan on using this file? If not, you can delete it.

Comment on lines +113 to +114
#if num_outstanding_rental_with_customer_id:
# return {"message" : f"Customer {customer_id} has outstanding rentals"}, 200
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove unused code

Comment on lines +26 to +32
customer_dict = {
"id" : customer.id,
"name" : customer.name,
"postal_code" : customer.postal_code,
"phone": customer.phone,
"register_at" : customer.register_at
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you created an instance method in your Customer model to handle this. I would use it here.

Comment on lines +73 to +78
if "name" in err.args:
return {"details" : f"Request body must include name."}, 400
if "postal_code" in err.args:
return {"details" : f"Request body must include postal_code."}, 400
if "phone" in err.args:
return {"details" : f"Request body must include phone."}, 400
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could create a function to handle these guard clauses dynamically and then use that function for customers and videos it could look something like

def validate_parameters(request_body, parameters):
           for attribute in parameters:
                if attribute not in request_body:
                       abort(make_response({"details": f"Request body must include {attribute}."}, 400))

Comment on lines +132 to +137
video_dict = {
"id" : video.id,
"title" : video.title,
"release_date" : video.release_date,
"total_inventory": video.total_inventory
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be moved to your video model and made an instance method

Comment on lines +243 to +244
num_of_rental = Rental.query.filter_by(video_id = video_id).count()
available_inventory = video.total_inventory - num_of_rental
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could add video_checkout_count as a column in your customer model then you could do something like

customer.videos_checked_out_count += 1
video.available_inventory -= 1

@tgoslee
Copy link

tgoslee commented Nov 28, 2021

Good Job Andrea and Elly! I liked some of the extra functions you all added and the use of try/except clauses. I added some comments on using instance methods you already created and refactoring some of your code. Let me know if you have questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants