-
Notifications
You must be signed in to change notification settings - Fork 26
Riyo & Margaret #19
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?
Riyo & Margaret #19
Conversation
… validation and test to customer model for name
Video StoreWhat We're Looking For
Well done on this project, you two! FANTASTIC work on the sorting and pagination! The code is well-written, clean, and thorough. I have a few comments on small places where I think code could be refactored. Other than that, this is a great submission. Great work! |
| def checkout | ||
| movie = Movie.find_by(id: params[:movie_id]) | ||
| customer = Customer.find_by(id: params[:customer_id]) | ||
| rental = Rental.new(customer_id: params[:customer_id], movie_id: params[:movie_id], checkout_date: Date.today, due_date: Date.today + 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.
It might be nice to utilize strong params method for making a new Rental!
| def checkout | ||
| movie = Movie.find_by(id: params[:movie_id]) | ||
| customer = Customer.find_by(id: params[:customer_id]) | ||
| rental = Rental.new(customer_id: params[:customer_id], movie_id: params[:movie_id], checkout_date: Date.today, due_date: Date.today + 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.
Determining the default checkout_date and due_date might be interesting business logic to pull into a model method!
| elsif rental.save | ||
| customer.movies_checked_out_count += 1 | ||
| customer.save | ||
| movie.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.
Same as above-- the logic for incrementing movies_checked_out_count might be a good Customer instance method, and incrementing available_inventory might be good to pull into Movie
It ends up being a slim method, but I rather like the idea of this line looking like:
movie.check_out
and in movie.rb
def checkout
self.available_inventory -= 1
end| it "sorts by id when not given sort query params" do | ||
| Movie.destroy_all | ||
| movie1 = Movie.create(title: "Test 1", inventory: 1) | ||
| movie1.id = 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.
Hm, it would be nice if this test could be refactored to not rely on manually setting movie1's id...
| { | ||
| title: "Titanic", | ||
| overview: "Romantic & sad", | ||
| release_date: 1997 - 12 - 19, |
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.
Watch out! I think that this literally evaluates to integer arithmetic, so this is 1966 hahaha
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.