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 - Georgina #27

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

Leaves - Georgina #27

wants to merge 3 commits into from

Conversation

geomsb
Copy link

@geomsb geomsb commented Aug 26, 2019

Grocery Store

Congratulations! You're submitting your assignment.

Comprehension Questions

Question Response
What is accomplished with raise ArgumentError? It stops the program and raises an Error when you receive an invalid input.
Why do you think we made the .all & .find methods class methods? Why not instance methods? because they are related to the class in general and we used them to get information from the class. Instance methods are used when we need specific information about objects or instances.
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? Customer was like planet and order was like solar system. Order has a one-to-many relation with customer.
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 customer, you can find the specific information about each customer. In order you can find the description of each order, the description includes the id of the customer. In Oder we used Customer.find to get the customer information and create a new order.
Did the presence of automated tests change the way you thought about the problem? How? Yes, in Wave 1, I used my own names and I got errors and then I realized that I needed to change some of the names because they were related. I learned that is important to review the test first before writing your program because some times you don't think in all the possible situations.

@geomsb geomsb changed the title Wave 1 and wave 2 Leaves - Georgina Aug 26, 2019
@geomsb
Copy link
Author

geomsb commented Aug 29, 2019

I realized that I didn't do the tests for the wave 2. I added the tests.

@dHelmgren
Copy link

Grocery Store

What We're Looking For

Feature Feedback
Baseline
Answered comprehension questions Yes
Used Git Regularly I would expect to see more commits on a project of this size.
Wave 1
All provided tests pass yes
Using the appropriate attr_ for instance variables mostly, see comment
Wave 2
All stubbed tests are implemented fully and pass yes
Used CSV library only in .all (not in .find) yes
Appropriately parses the product data from CSV file in Order.all yes
Order.all calls Customer.find to set up the composition relation yes
Additional Notes Solid work. Check my comments to see what can be improved, and make sure that you don't leave in puts statements in the future!


def self.all(file_name = 'data/customers.csv')
customers = []
clients = CSV.read(file_name, headers: true).map(&:to_h)

Choose a reason for hiding this comment

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

It's worth thinking about how you would solve this problem if you couldn't edit the csv to have headers.

attr_accessor :products, :customer, :fulfillment_status

def initialize(id, products, customer, fulfillment_status = :pending)
puts fulfillment_status

Choose a reason for hiding this comment

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

You need to take out puts statements before you submit! This is debugging code.

@@ -0,0 +1,62 @@
class Order
attr_reader :id
attr_accessor :products, :customer, :fulfillment_status

Choose a reason for hiding this comment

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

do all of these need to be accessors?

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