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

Ari and Kirsten #18

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

Ari and Kirsten #18

wants to merge 62 commits into from

Conversation

kanderson38
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. Initially, we didn't have a relationship between Customer and Movie, because there was no obvious link between the two, but once rentals were introduced, it was necessary to connect rentals with customer and movies.
What would be the Big-O time complexity of your /customers & /movies endpoints? What does the time complexity depend on? Explain your reasoning. We implemented the optional query parameters, so the data is being sorted, which has an O(n log n) time complexity, where n is the number of customers or 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. O(m + n + p), where m is the number of movies, n is the number of rentals, and p is the number of customers. We have to search through each of these lists linearly, so each needs its own variable.
Describe a set of positive and negative test cases you implemented for a model. For Movie, we tested that the movie would be valid with a title, and that it would not be valid without a title.
Describe a set of positive and negative test cases you implemented for a controller. Negative test: It renders an error message if there is not enough inventory to check out; Positive: If there was sufficient inventory, it would change the available inventory of the checked out movie.
How does your API respond when bad data is sent to it? If you try to create a movie and do not provide a title, the program renders a JSON error.
Describe one of your custom model methods and why you chose to wrap that functionality into a method. We didn't create any custom model methods because we didn't have a lot of business logic and it would have required more lines of code to break out that functionality into a different method.
Do you have any recommendations on how we could improve this project for the next cohort?
Link to ERD
MVIMG_20190517_133558

Summary |

@dHelmgren
Copy link

Video Store

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Good
Comprehension questions Breaking things out into a Model Method woulda been the right choice tho. :P
General
Business Logic in Models No, see comment.
All required endpoints return expected JSON data no
Requests respond with appropriate HTTP Status Code no
Errors are reported yes
Testing
Passes all Smoke Tests no
Model Tests - all relations, validations, and custom functions test positive & negative cases Good
Controller Tests - URI parameters and data in the request body have positive & negative cases See comments, kinda dropped the ball here.
Overall It seems to me like a small but crucial bit of learning re: postman was missed here. I think it's a pretty simple mistake with how you ran the smoke tests that probably caused these problems, because I see that the data is all there, just not formatted the way the tests were expecting. Besides that, there is no good reason to leave business logic in your controller! Not even line length! :P Finally, make sure you follow the testing format that we gave you to the T for each test. It might have helped you catch some of these mistakes.

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

if customer
render status: :ok, json: customer.as_json(only: [:id, :name, :registered_at, :postal_code, :phone, :movies_checked_out_count])

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.

movie.available_inventory -= 1
end

if rental.save

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.

You're not optimizing for lines of code, you're optimizing for a process that is easy to test and easy to read, and when you mix from different pots like this, both are compromised.

it "still returns JSON if ID is invalid" do
get customer_path(-1)
expect(response.header["Content-Type"]).must_include "json"
end

Choose a reason for hiding this comment

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

doesn't look like you check the fields!

it 'should get the correct path with query title' do
assert_recognizes({ controller: 'movies', action: 'index', sort: 'title' }, '/movies', sort: 'title')

get movies_path, params: { sort: 'title' }

Choose a reason for hiding this comment

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

is it an array? are the fields correct?

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