-
Notifications
You must be signed in to change notification settings - Fork 27
Time - Denisse & Angela #19
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?
Conversation
More customer file
Added model test for customer
…to include video changes
kaidamasaki
left a comment
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.
Excellent job! Your code is of extremely high quality and I've really only got minor suggestions for improvement.
| def index | ||
| @customers = Customer.order(:name) | ||
| render json: @customers.to_json( | ||
| :only => [:id, :name, :registered_at, :postal_code, :phone], :methods => [:videos_checked_out_count]), status: :ok |
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.
Great job using :methods here!
| validates :name, :address, :city, :state, :postal_code, :phone, presence: true | ||
|
|
||
| def videos_checked_out_count | ||
| return self.rentals.where(returned_on: nil).count |
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.
I'm impressed that you did all of this with Active Record methods instead of falling back on select and length!
| def valid_availability | ||
| if total_inventory.nil? | ||
| errors.add(:available_inventory, "bad data error") | ||
| end | ||
| end |
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 redundant with the presence: true for total_inventory.
| belongs_to :video | ||
| belongs_to :customer | ||
|
|
||
| validate :inventory_available, on: :create |
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 could have been replaced with:
| validate :inventory_available, on: :create | |
| validates :available_inventory, numericality: { greater_than: 0 } |
Video Store APIMajor Learning Goals/Code Review
Functional Requirements
Overall Feedback
Comprehension QuestionsThese are just for your learning and aren't something you're graded on:
If you're sorting by something with an index (like the id) this will actually be O(n) because of database optimizations.
The lookup here is going to be O(log(n)) because the find_by has to use the database indexes. (O(log(n)) is very fast though so is not something I would worry about.) Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
|
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.