Skip to content

Conversation

@paulentine
Copy link

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

Question Answer
Explain how you came up with the design of your ERD, based on the seed data. We used the seed data as attributes for the models in our ERD
What would be the Big-O time complexity of your /customers & /movies endpoints? What does the time complexity depend on? Explain your reasoning. O(n) where n is the number of customers / movies.
What is the Big-O time complexity of the POST /rentals/check-in endpoint? What does the time complexity depend on? Explain your reasoning. The Big-O time complexity depends on the time it takes to query and find the rental, customer & movie in the database and to update customer & movie fields & to destroy the rental.
Describe a set of positive and negative test cases you implemented for a model. For Customer: A positive test case is where everything is valid (all parameters required are present). Negative test cases were set by changing required parameters to nil, one at a time.
Describe a set of positive and negative test cases you implemented for a controller. Customer controller index positive test: will return all the customers. Negative test: Will load even with no customer.
How does your API respond when bad data is sent to it? Our API responds with bad_request for POST actions and not_found for GET actions. We then return JSON in a predictably structured error message format (array of errors, where the key of each error is the bad parameter).
Describe one of your custom model methods and why you chose to wrap that functionality into a method. We have none ¯_(ツ)_/¯
Do you have any recommendations on how we could improve this project for the next cohort? It's great as is :)
Link to Trello https://trello.com/b/Li0MyVw0/videostoreapi
Link to ERD https://i.ibb.co/mvSrDYS/erd.jpg
Summary Fun project!

@tildeee
Copy link

tildeee commented May 23, 2019

Video Store

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene x
Comprehension questions x
General
Business Logic in Models I think there's a lot of opportunity for this!
All required endpoints return expected JSON data x
Requests respond with appropriate HTTP Status Code x
Errors are reported x
Testing
Passes all Smoke Tests x, well done with the optionals!
Model Tests - all relations, validations, and custom functions test positive & negative cases x
Controller Tests - URI parameters and data in the request body have positive & negative cases x
Overall

Fantastic work on this project, you two!

The project is well done. I especially like the coverage and the organization of the tests!

I definitely think there are some compelling ways to refactor business logic into the models. I've made some comments.

Other than that, great work overall!

rental = Rental.new(rental_params)
rental.save

if rental.movie && rental.customer
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice conditional check!

rental.movie.save
rental.customer.movies_checked_out_count += 1
rental.customer.save
rental.due_date = rental.created_at + 7
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be really interesting to move most of this logic into an instance method in Rental. Imagine rental having a method named checkout:

def checkout
  self.movie.available_inventory -= 1
  self.customer.movies_checked_out_count += 1
  self.due_date = self.created_at + 7
end

and then the lines about rental.movie.save etc stayed in here. I feel that it's a lot cleaner!

customer = Customer.find_by(id: rental_params[:customer_id])
movie = Movie.find_by(id: rental_params[:movie_id])
rental = Rental.find_by(customer_id: rental_params[:customer_id], movie_id: rental_params[:movie_id])
if customer && movie && rental
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

if customer && movie && rental
rental.destroy
movie.update(available_inventory: movie.available_inventory + 1)
customer.update(movies_checked_out_count: customer.movies_checked_out_count - 1)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if they are slim methods, it might be cool to have a small method in movie that named update_inventory that held the logic about incrementing its inventory. Same for customer

end

describe "relationships" do
describe "rentals, has_many" do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how these tests are organized!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants