Skip to content

Conversation

@Kimberly-Fasbender
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 looked at the seed data and figured out what models we may need, and the attributes of those models, and then figured out how they would relate to one another.
What would be the Big-O time complexity of your /customers & /movies endpoints? What does the time complexity depend on? Explain your reasoning. The index and show endpoints would be O(n), and the create endpoint would be O(1). It depends on the size of instances stored in your database.
What is the Big-O time complexity of the POST /rentals/check-in endpoint? What does the time complexity depend on? Explain your reasoning. O(n) because it needs to iterate through all of the rentals to find the right one. The amount of time that it took would depend on the amount of rentals that were stored in the database.
Describe a set of positive and negative test cases you implemented for a model. For movie a positive test case we implemented was that a movie could have many rentals. A negative test case we implemented for movie was that a movie could also have zero rentals.
Describe a set of positive and negative test cases you implemented for a controller. A positive test case that we implemented for the movies controller was that it should create a new movie given valid data, and then a negative test case was that it returns an error when trying to create a movie given invalid data.
How does your API respond when bad data is sent to it? It gives a status of :bad_request, and returns a hash with an errors key, and the error messages as the value.
Describe one of your custom model methods and why you chose to wrap that functionality into a method. We made available_inventory a method, which returns the amount that is currently available for a given movie. We wrapped it in a method because it's business logic and doesn't belong in the controller!
Do you have any recommendations on how we could improve this project for the next cohort? Nope.
Link to Trello This project didn't seem complex/lengthy enough to necessitate one, and the project guidelines didn't specify to use one. So we used more verbal communication and pair programming to organize.
Link to ERD https://www.lucidchart.com/documents/edit/e84c2613-66ea-4d9e-b8f9-2083185c6240/0
Summary One fine day a Socket and a Port decided to build their very own API, and they did, and it was awesome! The end.

nidhiparixitpatel and others added 30 commits May 14, 2019 12:50
Kimberly-Fasbender and others added 28 commits May 15, 2019 13:19
removing inventory adjustments from check-in action
@droberts-sea
Copy link

Video Store

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene yes
Comprehension questions yes
General
Business Logic in Models some - see inline
All required endpoints return expected JSON data yes
Requests respond with appropriate HTTP Status Code yes
Errors are reported yes, but see inline
Testing
Passes all Smoke Tests yes
Model Tests - all relations, validations, and custom functions test positive & negative cases mostly - missing tests for Customer#movies_checked_out_count
Controller Tests - URI parameters and data in the request body have positive & negative cases yes - This coverage is quite thorough. If you had moved some of the checkin/checkout logic to the model, you would be able to test it there (which is a little easier) and these tests might be able to be simplified somewhat.
Overall Great job overall. I'm particularly happy with your test coverage. I do see some work for growth around identifying and isolating model methods. This won't be quite as straightforward to practice in JavaScript since we'll be so focused on display logic, but there will definitely be opportunities to isolate business logic. Other than that I like what I see - keep up the hard work.

def index
customers = Customer.all
render json: customers.as_json(only: [:id, :name, :registered_at, :address, :city, :state, :postal_code, :phone], methods: :movies_checked_out_count), status: :ok
end

Choose a reason for hiding this comment

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

I would probably break like 5 across multiple lines for readability.

if movie.save
render json: { id: movie.id }, status: :ok
else
render json: { errors: movie.errors.messages }, status: :bad_request

Choose a reason for hiding this comment

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

The error response on line 22 doesn't match the one in show above (line 12). That one has ok and messages as the keys, but this one sends errors. Best practice is to pick one error reporting format and stick with it.

customer = Customer.find_by(id: params[:customer_id])
error = "Customer not found" if customer == nil

movie = Movie.find_by(id: params[:movie_id])

Choose a reason for hiding this comment

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

You have a bunch of business logic here! Would it be possible to encapsulate this in a model method? Something like

Rental.check_out(customer_id, movie_id)

might be a good interface. That would make this logic easier to test as well.


def movies_checked_out_count
return Rental.where(check_in_date: nil).count
end

Choose a reason for hiding this comment

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

I love that you made Customer#movies_checked_out_count and Movie#available_inventory methods instead of storing them in the database. That way there's no way to forget to update these values.

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