-
Notifications
You must be signed in to change notification settings - Fork 26
Ngoc & Sarah #22
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?
Ngoc & Sarah #22
Conversation
…dded testing for movie controller - index method
…fixed tests accordingly
…iated test and customers controller
Video StoreWhat We're Looking For
Well done on this project, you two! The project submission is great-- not only is it well-organized, but I see places of the code being very elegant. Nice work! I'm particularly impressed with the code style in pulling the business logic out. I've made a few comments of how to refactor and suggestions to get better. Other than that, great work overall. |
| @@ -0,0 +1,37 @@ | |||
| require "pry" | |||
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.
Don't forget to get rid of this before submission! If anything, you likely wouldn't need to have this line, since pry is an included gem in the Gemfile
| rental = Rental.new(movie_id: @movie.id, customer_id: @customer.id) | ||
| cur_date = Date.today | ||
| rental.checkout_date = cur_date | ||
| rental.due_date = cur_date + 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.
Maybe this above work (setting rental's checkout_date and due_date) can also be pulled into an instance method on Rental
|
|
||
| has_many :rentals | ||
|
|
||
| def self.checkout_inventory(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.
Hm, maybe instead of making this a class method that takes in an instance of Movie as a parameter, it would work to make this an instance method. Then you would change the lines below to self.available_inventory
| def self.checkout_inventory(movie) | ||
| if movie.available_inventory >= 1 | ||
| movie.available_inventory -= 1 | ||
| movie.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.
This is cool! Because of Ruby's implicit return, this method will return true if the movie saved successfully, or false if it didn't. Nice!
| end | ||
| end | ||
|
|
||
| def self.checkin_inventory(movie, 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.
As mentioned above, maybe this can be an instance method that only takes in customer
| self.save | ||
| end | ||
|
|
||
| def self.checkin_movies_count(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.
Instead of making this a class method on Customer that takes in an instance, consider making this an instance method
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.