-
Notifications
You must be signed in to change notification settings - Fork 26
Socksports - Sav & Pauline #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 StoreWhat We're Looking For
Fantastic work on this project, you two! The project is well done. I especially like the coverage and the organization of the tests! I definitely think there are some compelling ways to refactor business logic into the models. I've made some comments. Other than that, great work overall! |
| rental = Rental.new(rental_params) | ||
| rental.save | ||
|
|
||
| if rental.movie && rental.customer |
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.
Nice conditional check!
| rental.movie.save | ||
| rental.customer.movies_checked_out_count += 1 | ||
| rental.customer.save | ||
| rental.due_date = rental.created_at + 7 |
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 think it would be really interesting to move most of this logic into an instance method in Rental. Imagine rental having a method named checkout:
def checkout
self.movie.available_inventory -= 1
self.customer.movies_checked_out_count += 1
self.due_date = self.created_at + 7
endand then the lines about rental.movie.save etc stayed in here. I feel that it's a lot cleaner!
| customer = Customer.find_by(id: rental_params[:customer_id]) | ||
| movie = Movie.find_by(id: rental_params[:movie_id]) | ||
| rental = Rental.find_by(customer_id: rental_params[:customer_id], movie_id: rental_params[:movie_id]) | ||
| if customer && movie && rental |
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.
Nice!
| if customer && movie && rental | ||
| rental.destroy | ||
| movie.update(available_inventory: movie.available_inventory + 1) | ||
| customer.update(movies_checked_out_count: customer.movies_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.
Even if they are slim methods, it might be cool to have a small method in movie that named update_inventory that held the logic about incrementing its inventory. Same for customer
| end | ||
|
|
||
| describe "relationships" do | ||
| describe "rentals, has_many" do |
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 like how these tests are organized!
Video Store API
Congratulations! You're submitting your assignment!
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.
Comprehension Questions
POST /rentals/check-inendpoint? What does the time complexity depend on? Explain your reasoning.bad_requestfor POST actions andnot_foundfor GET actions. We then return JSON in a predictably structured error message format (array of errors, where the key of each error is the bad parameter).