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

Daniela Sanchez - Leaves #28

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

Daniela Sanchez - Leaves #28

wants to merge 4 commits into from

Conversation

dnsanche
Copy link

Grocery Store

Congratulations! You're submitting your assignment.

Comprehension Questions

Question Response
What is accomplished with raise ArgumentError? To handle incorrect outputs by printing a message about why the error was generated.
Why do you think we made the .all & .find methods class methods? Why not instance methods? Because these methods allow to look the data for all the class instead of for an specific instance, which was helpful when you didn't know which was the instance to look for.
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? One order, can have only one customer, but one customer can have many orders, so this would be one to many relationship.
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? The CSV information represented in rows which are Id's and each row has information which is separated by commas. This is not very easy to analyze as you need to know the order of each element. In my program, customer and order are a class, with methods to represent behaviors, so each customer and order object can be analyzed easily.
Did the presence of automated tests change the way you thought about the problem? How? Yes, I had to first see the test and then modify my code to make it to work.

@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 for the most part, 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 on this! See my comments for some improvements for the future.


def self.all
#CSV is an array of Hashes were each hash is a customer.
csv = CSV.read('/Users/dnsanche/ada/week3/grocery-store/data/customers.csv', 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.

you can't submit code that has the absolute path to github, because I don't have a folder called 'dnsanche'! Use the relative path moving forward.

csv.each do |customer|
id = customer["id"].to_i
email = customer["email"]
address = {

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 do this if you couldn't add headers to the '.csv' file.

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 you need attr_accessor for all of these?

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