Skip to content
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

Heather (Ports) and Jansen (Sockets) #29

Open
wants to merge 32 commits into
base: master
Choose a base branch
from

Conversation

JansenMartin
Copy link

@JansenMartin JansenMartin commented May 17, 2019

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 thought about Rentals as being similar to Votes in MediaRanker. Just as Votes required a User and a Work, a Rental would require a Customer and a Movie. With that in mind, drawing the ERD based off of the seed data was pretty straightforward.
What would be the Big-O time complexity of your /customers & /movies endpoints? What does the time complexity depend on? Explain your reasoning. The time complexity for these endpoints is O(n), where n stands for the number of customers or movies inside of the database. The reason is because each endpoint results in a list of its respective database entries. The amount of time it takes to return this list hinges on how many entries are in the 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. At first, we wanted to say O(n), where n stands for the number of movies, customer AND rentals inside of the database. However, we implement a .order function in our check_in action. We don't totally know what type of sort Active Record implements, but..we think that this would ultimately change the time complexity to O(n log n).
Describe a set of positive and negative test cases you implemented for a model. For the movie model, we implemented a custom "decrease_inventory". We checked that it would successfully decrease the inventory of a movie when called. We also checked to make sure that it wouldn't decrease the inventory if it was already at 0.
Describe a set of positive and negative test cases you implemented for a controller. We checked that movies#show would get a movie with the required fields in JSON. We also checked that it would return not_found for a bogus movie.
How does your API respond when bad data is sent to it? It renders some sort of JSON message informing the user what went wrong, as well as a status code such as not_found or bad_request.
Describe one of your custom model methods and why you chose to wrap that functionality into a method. We made a "decrease_inventory" movie model method to use in our rentals controller for the check_out action. We did this because it felt appropriate for an instance of a movie to keep track of its own inventory. In other words, we left movie-related business logic inside the movie model.
Do you have any recommendations on how we could improve this project for the next cohort?
Link to Trello We paired the entire time, and we were having so much fun playing TDD ping pong that we didn't use the Trello board.
Link to ERD Here!
Summary

JansenMartin and others added 30 commits May 14, 2019 13:55
@dHelmgren
Copy link

Video Store

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Good
Comprehension questions Good
General
Business Logic in Models Not so much, see comment.
All required endpoints return expected JSON data Yes
Requests respond with appropriate HTTP Status Code Yes
Errors are reported
Testing
Passes all Smoke Tests Yes
Model Tests - all relations, validations, and custom functions test positive & negative cases yes
Controller Tests - URI parameters and data in the request body have positive & negative cases yes
Overall Solid work on this! The biggest thing I want to call attention to is moving more of that business logic into the model!


def zomg
render json: { message: "It works!" }
end

Choose a reason for hiding this comment

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

Oh goshers. You didn't need to leave this in. :P

customer = Customer.find_by(id: params[:customer_id])

if movie.nil? && customer.nil?
render status: :not_found, json: { errors: ["Customer with id #{params[:customer_id]} was not found.", "Movie with id #{params[:movie_id]} was not found"] }

Choose a reason for hiding this comment

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

Small style note: not all text editors wrap lines for you. This line is so long that on GitHub I have to scroll horizontally to see all the pieces. You can make this easier to read by putting a newline after any given comma in a statement.

class RentalsController < ApplicationController
def check_out
movie = Movie.find_by(id: params[:movie_id])
customer = Customer.find_by(id: params[:customer_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.

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