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

Branches - Brianna K #40

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

Branches - Brianna K #40

wants to merge 6 commits into from

Conversation

brikemp
Copy link

@brikemp brikemp commented Aug 26, 2019

Grocery Store

Congratulations! You're submitting your assignment.

Comprehension Questions

Question Response
What is accomplished with raise ArgumentError? With the raise ArgumentError syntax, the program stops executing and a message can be passed to the user letting them know what the issue is. In Orders, the raise ArgumentError, stops a new instance of Order from being created and lets the user know that the fulfillment_status entered was invalid.
Why do you think we made the .all & .find methods class methods? Why not instance methods? I think we made .all and .find class methods so that they could refer to multiple/all instances of the Order and Customer classes - instance methods are more restrictive and wouldn't have allowed us to do this
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? The relationship between Order and Customer is one-to-many. While some customers in the data presented have multiple orders and some just have one the best way to represent the data is one-to-many. This is similar to the Solar System project because one solar system could have 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 csv file the relationship is created by placing a customer id in the order.csv file. This might potentially be different in the program because the customer would need to be created first, so that instance of customer could be used during the order initialization. Otherwise, we would have to continue to use the Customer id, which is less flexible and would require using Customer.find to get any info about the customer.
Did the presence of automated tests change the way you thought about the problem? How? The presence of the tests did change the way I thought about the problem at times. While writing the program I would check the test results and decide to switch to different methods when it was clear something was going to keep failing. The test are really helpful for making sure you're going down the right path and alerting you when you need to switch up your strategy.

all_customers = []
customer_data = CSV.open('data/customers.csv', headers:false).map(&:to_a)
customer_data.each do |customer|
parameter1 = customer[0].to_i

Choose a reason for hiding this comment

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

This is another example where I'd suggest coming up with more descriptive variable names.

@jmaddox19
Copy link

Grocery Store

What We're Looking For

Feature Feedback
Baseline
Answered comprehension questions X
Used Git Regularly I would recommend breaking commits down into smaller pieces than waves usually are. That said, the commits you have look great and the messages are clear!
Wave 1
All provided tests pass X
Using the appropriate attr_ for instance variables X
Wave 2
All stubbed tests are implemented fully and pass X
Used CSV library only in .all (not in .find) X
Appropriately parses the product data from CSV file in Order.all X
Order.all calls Customer.find to set up the composition relation You associated Order with Customer using the customer_id in Order rather than actually initializing an instance of customer. Because of this, the user of your system would need to do extra work to actually access an instance of a customer from the order. I assume this was just a misunderstanding of the instructions but if it is due to a low level of confidence with composition, I'm happy to chat with you about that in our 1:1 next week (if not sooner).
Additional Notes Great job overall! Definitely want to make sure you're able to apply composition since you didn't end up doing so in this project.

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