Conversation
Video StoreWhat We're Looking For
|
| def show | ||
| @customer = Customer.find_by(id: cust_params[:id]) | ||
| if @customer.nil? | ||
| render json: { |
There was a problem hiding this comment.
While this works, you don't typically use strong params for URL parameters.
| def create | ||
| @movie = Movie.new(movie_params) | ||
| @movie.save! | ||
| render :create, status: :ok |
There was a problem hiding this comment.
Using bang (!) methods works great for the console or scripts like the seed file, but in your actual controller actions you should be calling the non-bang version and checking the result. As-is, this will produce an exception rather than a helpful error message, which is not a good user experience.
Something like this would be more appropriate:
@movie = Movie.new(movie_params)
if @movie.save
render :create, status: :ok
else
render json: { errors: @movie.errors.messages }, status: :bad_request
end| if @movie.nil? | ||
| render json: { | ||
| errors: { | ||
| id: ["No movie with ID #{movie_params[:id]}"] |
There was a problem hiding this comment.
This is a good example of well-done error handling.
| customer_id = check_params[:customer_id] | ||
| movie_id = check_params[:movie_id] | ||
|
|
||
| find_customer_and_movie(customer_id, movie_id) |
There was a problem hiding this comment.
I like the idea of moving the shared logic to find the customer and the movie into a helper method. Why not take it a step further and make it a controller filter?
|
|
||
| if @rental.save | ||
|
|
||
| Rental.process_checkout(@customer, @movie) |
There was a problem hiding this comment.
This dance of checking the dependencies, creating the rentals, and then updating the dependent models is pretty complicated - it might make sense to encapsulate the whole thing in a model method. One thing that might help is using a transaction - go look this up!
| def self.check_dependencies(customer, movie) | ||
| if customer.nil? | ||
| return { | ||
| customer_id: ["No such customer"] |
There was a problem hiding this comment.
Since the customer and movie are about to be associated with an instance of Rental, it might make sense to make this an instance method.
|
|
||
| it "Creates a new movie" do | ||
| # this is taking the count before and after | ||
| before_count = Movie.count |
| describe Rental do | ||
|
|
||
| describe "relations" do | ||
| it "must respond to customer" do |
There was a problem hiding this comment.
You've done such a good job of encapsulating your logic in the model! One of the big advantages of that is that you can test the logic in isolation here, but you're not doing that.
4 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