-
Notifications
You must be signed in to change notification settings - Fork 27
Space - Liv & Faezeh #11
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
Video Store APIMajor Learning Goals/Code Review
Functional Requirements
Overall Feedback
Additional FeedbackGreat work y'all! Seriously, it's a yellow because of the specific code review points the rubric touches on based on the new learning objectives, but regardless y'all built a fully functional API and you should be proud of that! Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
|
| new_rental = Rental.new(rentals_params) | ||
| new_rental.checkout_date = Date.today | ||
| new_rental.due_date = Date.today + 7.days | ||
| new_rental.return_date = nil |
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.
This is stuff that would be appropriate to move to a method on the rental model.
| 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.
I know this is only two lines, but it could be one line in the controller if there were a method to do this in the video model.
| 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.
Same here for the customer model
| end | ||
| end | ||
|
|
||
| def checkin |
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 are opportunities to move things to models in this method as well.
As a general rule, any time I see more than one line of code that isn't directly related to the user-control-flow of the application, I ask myself: "could I pull this code out into a model?
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.
Feel free to reach out if that isn't totally clear!
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.