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

Leaves - Mariya #44

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

Leaves - Mariya #44

wants to merge 5 commits into from

Conversation

M-Burr
Copy link

@M-Burr M-Burr commented Aug 26, 2019

Grocery Store

Congratulations! You're submitting your assignment.

Comprehension Questions

Question Response
What is accomplished with raise ArgumentError? To alert the user or programmer that that something is wrong.
Why do you think we made the .all & .find methods class methods? Why not instance methods? So we can search for specific orders or customers across the class (ie., all the orders/customers). Instance methods would only give us access to information about that specific instance of a customer or order.
Think about the relation between Order and Customer. Is this relation one-to-one, one-to-many, or something else? How does that compare to the Solar System project? It is a one to many relation (one customer has many orders). The solar system was also one to many (one solar system for many planets).
How is the relation between Order and Customer tracked in the CSV file? How is it tracked in your program? Why might these be different? In the order CSV, customer is tracked using a customer id. In the program, order tracks customers by customer instances (which contain all the information about the customer).
Did the presence of automated tests change the way you thought about the problem? How? In some ways it made it more difficult and I ran into errors (e.g., not using the same variable names as the test), but it also helped me feel more confident about the final result of my code.


def self.find_by_customer(customer_id)
customer_orders = []
Order.all.each do |order|

Choose a reason for hiding this comment

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

This looks good! In case you want something shorter that does exactly what you implemented, you might want to check out: https://ruby-doc.org/core-2.6.1/Enumerable.html#method-i-find



it "Returns nil for an order that doesn't exist" do
no_order = Order.find(101)

Choose a reason for hiding this comment

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

It might be nice to give a name to the value 101, like non_extant_order_id = 101

@beccaelenzil
Copy link

Grocery Store

What We're Looking For

Feature Feedback
Baseline
Answered comprehension questions check -- great, on point responses
Used Git Regularly It would be good to have more few meaningful commits.
Wave 1
All provided tests pass check
Using the appropriate attr_ for instance variables check
Wave 2
All stubbed tests are implemented fully and pass check
Used CSV library only in .all (not in .find)
Appropriately parses the product data from CSV file in Order.all check
Order.all calls Customer.find to set up the composition relation check
Additional Notes Good job overall! Your code is well-organized and easy to read, and does a good job of solving the problem at hand. I've left a few inline comments for you to review, but it's clear that the learning goals for this assignment were met. Keep up the hard work!

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.

2 participants