Skip to content

Time - Olga & Quin #5

Open
quinqu wants to merge 41 commits intoAda-C13:masterfrom
quinqu:master
Open

Time - Olga & Quin #5
quinqu wants to merge 41 commits intoAda-C13:masterfrom
quinqu:master

Conversation

@quinqu
Copy link

@quinqu quinqu commented May 28, 2020

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

Prompt Response
Explain how you came up with the initial design of your ERD, based on the seed data and reading through the API endpoints We created initial tables with fields equal to those that we have in json file. Later we had to add videos_checked_out_count column to seed the database.
What would be the Big-O time complexity of your /customers & /videos endpoints? What does the time complexity depend on? Explain your reasoning. For an index action we need to output all videos and customers the database has, so it’s going to be O(n).
What is the Big-O time complexity of the POST /rentals/check-in endpoint? What does the time complexity depend on? Explain your reasoning. We believe that find_by method that is present in the check_in method in 3 different parts is going to be the most time-consuming part, and it makes the time complexity equal to O(n) as it goes to a db table to see if there’s a specific record there.
Describe a specific set of positive and negative test cases you implemented for a model. For self.inventory_check_in Rental model method we checked if the method result is true for the valid customer and video IDs, and if it’s changing the videos_checked_out_count and available_inventory values. Negative test cases included checking that the result is false if invalid customer and video is provided.
Describe a specific set of positive and negative test cases you implemented for a controller. For the rental model, we tested that a rental would not be created if video_inventory was < 1. We also tested that the rental would have a correct due date.
Broadly, describe how an API should respond when it handles a request with invalid/erroneous parameters. It should give back JSON with the errors
Describe one of your custom model methods and why you chose to wrap that functionality into a method. One of our model methods was for check out. We made sure that the customer and video were valid and that there was a video able to be checked out. In this method, we also decremented video availability and incremented the customer's videos_checked_out_count

@kaidamasaki
Copy link

Video Store API

Major Learning Goals/Code Review

Criteria yes/no, and optionally any details/lines of code to reference
Practices git with at least 10 small commits and meaningful commit messages ✔️
Understands the importance of how APIs handle invalid/erroneous data in their reflection questions ✔️
Practices Rails best practices and well-designed separation, and encapsulates business logic around check-out and check-in in models ✔️
Uses controller tests to ensure quality code for every route, and checks for response status and content, with positive cases and negative cases ✔️
Uses controller tests to ensure correctness for check out and check in requirement, and that checked out counts and inventories appropriately change ✔️

Functional Requirements

Functional Requirement yes/no
All provided smoke tests for Wave 1 pass ✔️
All provided smoke tests for Wave 2 pass ✔️

Overall Feedback

Overall Feedback Criteria yes/no
Green (Meets/Exceeds Standards) 3+ in Code Review && 2 in Functional Requirements ✔️
Yellow (Approaches Standards) 2+ in Code Review && 1+ in Functional Requirements, or the instructor judges that this project needs special attention
Red (Not at Standard) 0-1 in Code Review or 0 in Functional Reqs, or assignment is breaking/doesn’t run with less than 5 minutes of debugging, or the instructor judges that this project needs special attention

Comprehension Questions

We believe that find_by method that is present in the check_in method in 3 different parts is going to be the most time-consuming part, and it makes the time complexity equal to O(n) as it goes to a db table to see if there’s a specific record there.

Because you created indexes for these this actually winds up being a O(log(n)) operation, which is pretty efficient. It would be O(n) if you hadn't created indexes on your foreign keys. (Databases use B-Trees for things like indexes, hence the logarithmic search.)

Code Style Bonus Awards

Was the code particularly impressive in code style for any of these reasons (or more...?)

Quality Yes?
Perfect Indentation
Elegant/Clever
Descriptive/Readable
Concise
Logical/Organized

Copy link

@kaidamasaki kaidamasaki left a comment

Choose a reason for hiding this comment

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

Excellent job! I really like how you handled your check in/out. It's very clean and elegant.

I've got a few style notes but other than that everything looks great!

@@ -0,0 +1,12 @@
class Customer < ApplicationRecord
has_many :rentals
has_many :videos, :through => :rentals

Choose a reason for hiding this comment

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

Style: I'd probably update this line to match the ones below.

Suggested change
has_many :videos, :through => :rentals
has_many :videos, through: :rentals

belongs_to :customer
belongs_to :video

def self.inventory_check_out(rental)

Choose a reason for hiding this comment

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

Style: I would have made these instance methods since you pass an instance in anyway.

Comment on lines +14 to +16
return true
else
return false

Choose a reason for hiding this comment

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

I really like returning a boolean value to indicate success or failure here though.

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