Conversation
Gs/routes
Gs/customer model
completed movie model tests
relationship tests for customer and rental
Gs/custom methods
Customers movies controllers
added positive test for movie model
added available inventory to movies schema
passing smoke test for wave 2
Gs/controllers
Video StoreWhat We're Looking For
Well done on this project, you two! I'm really happy with how the controller tests and the model tests look. It has some really nice readable code style there. I think that the project could have been better with a better understanding of models. There were a lot of places that could be improved with better execution on:
I also saw some places of weird, messy code... and places that could be refactored to use nicer forms of code. It's getting to that point of the curriculum -- I fully believe y'all can do better code style! That being said, you two did very well on this project overall. Good work! |
|
|
||
| if rental.movie_avail?(rental_params[:movie_id]) | ||
| if rental.save | ||
| customer = Customer.find_by(id: rental.customer_id) |
There was a problem hiding this comment.
Hm, did you know that customer = Customer.find_by(id: rental.customer_id) is the same thing as customer = rental.customer, since Rental belongs to Customer?
| if rental.movie_avail?(rental_params[:movie_id]) | ||
| if rental.save | ||
| customer = Customer.find_by(id: rental.customer_id) | ||
| customer.movies_checked_out_count += 1 |
There was a problem hiding this comment.
This business logic would be better if moved to the model. Maybe a new instance method, like customer.increase_checkout?
| rental = Rental.new(rental_params) | ||
| rental.check_out = Date.today | ||
| rental.check_in = rental.check_out + 7 | ||
| rental.status = "unavailable" |
There was a problem hiding this comment.
All of this work on the new rental may be better as a model method
| customer.movies_checked_out_count += 1 | ||
| customer.save | ||
|
|
||
| movie = Movie.find_by(id: rental.movie_id) |
There was a problem hiding this comment.
As above, it would be better to utilize the fact that rental belongs to movie, and do movie = rental.movie
| customer.save | ||
|
|
||
| movie = Movie.find_by(id: rental.movie_id) | ||
| movie.available_inventory -= 1 |
There was a problem hiding this comment.
This business logic would be better if moved to the model. Maybe a new instance method, like movie.decrease_inventory?
| has_many :rentals | ||
|
|
||
| def movies_checked_out_count | ||
| total = 0 |
| def movies_checked_out_count | ||
| total = 0 | ||
| num_unavailable = 0 | ||
| num_unavailable = 0 |
| if rental.status == "unavailable" | ||
| num_unavailable += 1 | ||
| end | ||
| end |
There was a problem hiding this comment.
Nice each loop! Instead of an each loop, could we do something like Enumerables filter or count method?
| num_unavailable += 1 | ||
| end | ||
| end | ||
| return (movies_count = num_unavailable) |
There was a problem hiding this comment.
Hm, what does return (movies_count = num_unavailable) mean? Although it's a valid line of code, it's hard to read. What are you returning? Why would you need to do assignment to another variable?
| validates :movie, :customer, :check_out, :check_in, :status, presence: true | ||
|
|
||
| def movie_avail?(movie_id) | ||
| movie = Movie.find_by(id: movie_id) |
There was a problem hiding this comment.
This method has nothing to do with Rental! So I'm not convinced it needs to be an instance method on Rental. Should move this method to the Movie file?
| validates :movie, :customer, :check_out, :check_in, :status, presence: true | ||
|
|
||
| def movie_avail?(movie_id) | ||
| movie = Movie.find_by(id: movie_id) |
There was a problem hiding this comment.
Also, do we need to pass in a movie_id? If it's on the Movie, then it should reference itself. Otherwise, if it's on the Rental, then we should reference the movie through rental, since rental belongs to movie
| return true | ||
| else | ||
| return false | ||
| end |
There was a problem hiding this comment.
Could we simplify this code somehow? Consider:
if !movie.nil? && movie.inventory > 0
return true
end
return false3 participants
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.