-
Notifications
You must be signed in to change notification settings - Fork 25
Goeun&Semret - Edges - VideoStoreAPI #2
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
Hi Goeun and Semret! Y'all did a great job on this project! I think this code looks really nice and clean. I think that you all also did a really wonderful job with organizing code. I think you both did a really great job of moving code to the best places it could be, in both application logic (controller and model) and tests. Overall, the project passes all of the requirement, and it does so stylishly. Great work! |
| def movie_params | ||
| # 11 tests are failing and found out that we need to change the params from | ||
| # params.require(:movie) | ||
| # now 8 tests passed on postman |
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.
What is this comment in reference to so I can address it?
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.
Oh these were our note for ourselves but we forgot to delete it in the final version. All of our tests pass. Sorry for the confusion.
| rental.due_date = Date.today + 7 | ||
| if rental.save | ||
|
|
||
| rental.rent_movie(customer, movie) |
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! This! Method!
|
|
||
| def find_movie | ||
| return Movie.find_by(id: rental_params[:movie_id]) | ||
| 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.
These private methods are great! It ends up making the code look so clean when it's used!
|
|
||
| def rent_movie(customer, movie) | ||
| checked_out = customer.movies_checked_out_count | ||
| customer.update(movies_checked_out_count: checked_out +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.
Minor syntax thing: Ruby linters complain if in Ruby you don't put a space before 1 (it would prefer + 1). Same with the minus sign below on line 20
|
|
||
| inventory = movie.available_inventory | ||
| movie.update(available_inventory: inventory +1) | ||
| 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.
These two methods are so wonderful and perfect. I want to show them off! This is exactly what I was looking for: You pulled out this logic into the Rental, which had logic that it delegated to customer and movie. Nicely done.
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
Customer.allorMovie.all. The index methods (for both customer and movie) iterate through the array of hashes once and the time complexity would depend on how large n is.POST /rentals/check-inendpoint? What does the time complexity depend on? Explain your reasoning.rent_movieandreturn_movieboth with positive cases. We didn’t add a negative case for them because the way those methods would be implemented in our RentalsController would assure only valid data.