Nataliya - Time #22
Nataliya - Time #22
Conversation
…alidation to Rental model
…few bugs in testing
Video Store APIMajor Learning Goals/Code Review
Functional Requirements
Overall Feedback
Additional FeedbackComprehension QuestionsI'm just expanding on these because I know that you like detailed explanations.
Looking up on the primary key in a database is generally an O(log(n)) operation since database tables are stored as trees (and are typically organized by primary keys). However, the base of that log tends to be very large because they use a structure called a B-Tree. (Feel free to ask me more about B-Trees if the Wikipedia article is confusing.)
This is actually also a quite fast operation. Because you added indexes to your foreign keys the database actually created one additional B-Tree per reference which means looking up against each of these is a O(log(n)) operation which means that the total complexity of check-in is O(log(n)). (A nice thing about databases is that you don't have to tell them to use indexes, they just do automatically when you reference columns.) Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
|
kaidamasaki
left a comment
kaidamasaki
left a comment
There was a problem hiding this comment.
Great job!
Overall your code was quite clean and clear. There were a few little things I'd refresh on for your next project. It's probably worth taking another look at fixtures and model methods.
Your controllers were clearly defined and I appreciated how you split out your logic and especially how thorough your tests were.
| rental = Rental.new(customer_id: @customer.id, video_id: @video.id) | ||
|
|
||
| if rental.video.available_inventory == 0 | ||
| render json: { errors: ["No available copies of the video available"] }, status: :bad_request | ||
| return | ||
| end | ||
|
|
||
| if rental.save | ||
| rental.customer.videos_checked_out_count += 1 | ||
| rental.customer.save | ||
| rental.video.available_inventory -= 1 | ||
| rental.video.save | ||
|
|
||
| render json: { | ||
| customer_id: rental.customer_id, | ||
| video_id: rental.video_id, | ||
| due_date: rental.due_date, | ||
| videos_checked_out_count: rental.customer.videos_checked_out_count, | ||
| available_inventory: rental.video.available_inventory | ||
| }, status: :ok |
There was a problem hiding this comment.
Most of this logic should have been encapsulated into a model method to handle checking out.
| rental = Rental.new(customer_id: @customer.id, video_id: @video.id) | |
| if rental.video.available_inventory == 0 | |
| render json: { errors: ["No available copies of the video available"] }, status: :bad_request | |
| return | |
| end | |
| if rental.save | |
| rental.customer.videos_checked_out_count += 1 | |
| rental.customer.save | |
| rental.video.available_inventory -= 1 | |
| rental.video.save | |
| render json: { | |
| customer_id: rental.customer_id, | |
| video_id: rental.video_id, | |
| due_date: rental.due_date, | |
| videos_checked_out_count: rental.customer.videos_checked_out_count, | |
| available_inventory: rental.video.available_inventory | |
| }, status: :ok | |
| if video.available_inventory == 0 | |
| render json: { errors: ["No available copies of the video available"] }, status: :bad_request | |
| return | |
| end | |
| rental = Rental.new(customer: @customer, video: @video) | |
| if rental.check_out | |
| render json: { | |
| customer_id: rental.customer_id, | |
| video_id: rental.video_id, | |
| due_date: rental.due_date, | |
| videos_checked_out_count: rental.customer.videos_checked_out_count, | |
| available_inventory: rental.video.available_inventory | |
| }, status: :ok |
class Rental < ApplicationRecord
# Relationships, validations, etc...
def check_out(customer, video)
if rental.save
rental.customer.videos_checked_out_count += 1
rental.customer.save
rental.video.available_inventory -= 1
rental.video.save
return rental
else
return nil
end
end
endThere was a problem hiding this comment.
In a production application you would also probably want to use a transaction for safety when updating multiple tables like this, though we haven't taught you those (and don't as part of the curriculum due to a lack of time).
| rental.destroy | ||
| rental.customer.videos_checked_out_count -= 1 | ||
| rental.customer.save | ||
| rental.video.available_inventory += 1 | ||
| rental.video.save |
There was a problem hiding this comment.
Similar to above this should probably live in a rental.check_in method.
class Rental < ApplicationRecord
# ...
def check_in
customer.videos_checked_out_count -= 1
customer.save
video.available_inventory += 1
video.save
self.destroy
end
end| def require_customer | ||
| @customer = Customer.find_by(id: params[:customer_id]) | ||
| if @customer.nil? | ||
| render json: { errors: ["Not Found"] }, status: :not_found | ||
| end | ||
| end | ||
|
|
||
| def require_video | ||
| @video = Video.find_by(id: params[:video_id]) | ||
| if @video.nil? | ||
| render json: { errors: ["Not Found"] }, status: :not_found | ||
| end | ||
| end |
| before_save :default_values | ||
| def default_values | ||
| self.active = true if self.active.nil? | ||
| self.due_date = Date.today + 7.day if self.due_date.nil? | ||
| end |
There was a problem hiding this comment.
Good use of before_safe! I especially like that your default_values method checks to make sure not to overwrite existing values.
| before do | ||
| Customer.create( | ||
| name: "Evelynn", | ||
| registered_at: "Wed, 29 Apr 2015 14:54:14 UTC +00:00", | ||
| address: "13133 111th Dr Se", | ||
| city: "Monroe", | ||
| state: "WA", | ||
| postal_code: "98989", | ||
| phone: "425 425 44 44", | ||
| videos_checked_out_count: 0 | ||
| ) | ||
|
|
||
| Video.create( | ||
| title: "Cinderella", | ||
| overview: "After her father unexpectedly dies, young Ella (Lily James) finds herself at the mercy of her cruel stepmother (Cate Blanchett) and stepsisters, who reduce her to scullery maid. Despite her circumstances, she refuses to despair.", | ||
| release_date: "2015-03-06", | ||
| total_inventory: 5, | ||
| available_inventory: 5 | ||
| ) |
There was a problem hiding this comment.
This probably should have been part of your fixtures.
| @customer = Customer.find_by(name: "Evelynn") | ||
| @video = Video.find_by(title: "Cinderella") |
There was a problem hiding this comment.
You do additional find_bys for these later, which you don't need to do since they already exist in instance variables.
| it "increase the customer's videos_checked_out_count by one" do | ||
| expect(@customer.videos_checked_out_count).must_equal 0 | ||
|
|
||
| post rentals_path, params: @rental_data | ||
| customer = Customer.find_by(name: "Evelynn") | ||
| expect(customer.videos_checked_out_count).must_equal 1 | ||
| end |
There was a problem hiding this comment.
You could make this cleaner by using must_differ.
Also, it's a good idea to assert on the response code here as well.
| it "increase the customer's videos_checked_out_count by one" do | |
| expect(@customer.videos_checked_out_count).must_equal 0 | |
| post rentals_path, params: @rental_data | |
| customer = Customer.find_by(name: "Evelynn") | |
| expect(customer.videos_checked_out_count).must_equal 1 | |
| end | |
| it "increase the customer's videos_checked_out_count by one" do | |
| expect do | |
| post rentals_path, params: @rental_data | |
| end.must_differ "@customer.refresh.videos_checked_out_count", 1 | |
| must_respond_with :ok | |
| end |
| # For details on the DSL available within this file, see https://guides.rubyonrails.org/routing.html | ||
| resources :videos, only: [:index, :show, :create] | ||
| resources :customers, only: [:index] | ||
| resources :rentals, only: [:create, :destroy] |
There was a problem hiding this comment.
You didn't need these routes since you only interact with rentals using the custom routes below.
2 participants
Assignment Submission: Video Store API
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.
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.
Reflection
/customers&/videosendpoints? What does the time complexity depend on? Explain your reasoning.POST /rentals/check-inendpoint? What does the time complexity depend on? Explain your reasoning.