Myriam (Ports) and Evelynn (Sockets)#26
Myriam (Ports) and Evelynn (Sockets)#26evelynnkaplan wants to merge 35 commits intoAda-C11:masterfrom
Conversation
Video StoreWhat We're Looking For
Well done! Firstly, this project had many smoke tests that were failing. A lot of them actually get solved after a small change, which I have below in the comments. That being said, the code was overall good. There are still some failing tests that I haven't looked into, and I think there's opportunity to bring some logic into model methods. Also, I personally would think it's a better call to name the model Otherwise, well done on this project! |
| def checkout | ||
| customer_movie = CustomerMovie.new(customer_movie_params) | ||
| if customer_movie.save | ||
| checkout_movie = customer_movie.movie.title |
There was a problem hiding this comment.
Should we be setting checkout_movie to the value of its movie's title? This doesn't seem to break any tests, but it reads really strangely to me.
| checkout_movie = customer_movie.movie.title | ||
| customer_movie.checkout_date = Date.today | ||
| customer_movie.due_date = Date.today.next_week | ||
| customer_movie.movie.inventory -= 1 |
There was a problem hiding this comment.
I think it would be really cool if all of this work around modifying customer_movie were pulled into an instance method on CustomerMovie! Imagine a method on CustomerMovie named checkout called with checkout_movie.checkout:
def checkout
self.checkout_date = Date.today
self.due_date = Date.today.next_week
self.movie.inventory -= 1
end
| customer_movie = CustomerMovie.where(customer_id: customer_movie_params[:customer_id], movie_id: customer_movie_params[:movie_id]) | ||
| if customer_movie != [] | ||
| customer_movie[0].movie.inventory += 1 | ||
| customer_movie[0].status = "returned" |
There was a problem hiding this comment.
Similar to above: it may be best if these two lines about incrementing inventory and changing status were pulled into an instance method. Even if it's two lines long, it's still helpful!
| private | ||
|
|
||
| def movie_params | ||
| params.require(:movie).permit(:title, :overview, :release_date, :inventory) |
There was a problem hiding this comment.
In Rails when we used forms, params would come back populated with a nested structure that had movie in it. In this API, we won't have that nested structure, so we need to take out the .require('movie') bit. When we do this, then most of Wave 2 starts working :)
3 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.