-
Notifications
You must be signed in to change notification settings - Fork 27
Space - Leah Stephanie & June #18
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
base: master
Are you sure you want to change the base?
Conversation
…ideo, increasing video checked out count in customer
…ked out and inventory still not passing
Video Store APIMajor Learning Goals/Code Review
Functional Requirements
Overall Feedback
Additional FeedbackGreat work y'all! Such thorough testing! Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
|
| if customer.nil? || video.nil? | ||
| render json: { errors: ['Not Found']}, status: :not_found | ||
| return | ||
| end | ||
|
|
||
| if rental.nil? | ||
| render json: { errors: ['Rental not valid'] }, status: :bad_request | ||
| return | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love that you separated these out into two separate cases to give a more clear error message to the user!
| customer[:videos_checked_out_count] -= 1 | ||
| customer.save |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be slightly cleaner if these two lines were setup as a method in the customer model so that it could be only one line here.
| video[:available_inventory] += 1 | ||
| video.save |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here as above, but in the video model.
| else | ||
| render json: { errors: rental.errors.full_messages }, status: :bad_request | ||
| return | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There doesn't seem to be to be a way for the user's request to be bad at this point because the customer_id and the video_id have already both been verified.
Because of that, it could still be appropriate to have an else clause here but the only way it'd get executed is if the connection to the database failed. In that case, the more appropriate status would be an :internal_server_error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please let me know if this comment is unclear!
Assignment Submission: Video Store API
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.
If you didn't get to the functionality the question is asking about, reply with what you would have done if you had completed it.
Reflection
/customers&/videosendpoints? What does the time complexity depend on? Explain your reasoning.POST /rentals/check-inendpoint? What does the time complexity depend on? Explain your reasoning.