-
Notifications
You must be signed in to change notification settings - Fork 27
Time & Space - Corinna & Katie #9
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
…d registered_at to customers
Video Store APIMajor Learning Goals/Code Review
Functional Requirements
Overall Feedback
Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
|
beccaelenzil
left a comment
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.
Good work on this project! Your code is well written and well tested. The one main piece to note is that the logic for checking in and checking out a video should be in the model. There are ways to do this in the customer/video models, or the rental model. Please let me know if you have any questions. Keep up the hard work!
| }, status: :not_found | ||
| return | ||
| elsif rental.save | ||
| video.available_inventory -= 1 |
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.
Changing the counts is business logic that should go in the model. One way to think about whether it's business logic or not is to consider if you are manipulating the data. If you are, stick it in the model!
| errors: ["Not Found"], | ||
| },status: :not_found | ||
| elsif rental | ||
| rental.customer.videos_checked_out_count -= 1 |
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.
Similar to above, this business logic should go in the model.
| video = Video.find_by(id: rental[:video_id]) | ||
| customer = Customer.find_by(id: rental[:customer_id]) | ||
|
|
||
| expect(video.available_inventory).must_equal inventory + 1 |
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.
Consider using must_differ as above.
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./customers&/videosendpoints are O(n), where n is the length of the collection.POST /rentals/check-inendpoint? What does the time complexity depend on? Explain your reasoning.POST /rentals/check-inendpoint is O(n), where n is the length of the rentals collection.