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 - Amal #26

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

Branches - Amal #26

wants to merge 11 commits into from

Conversation

ashassan
Copy link

Grocery Store

Congratulations! You're submitting your assignment.

Comprehension Questions

Question Response
What is accomplished with raise ArgumentError? It is used to stop the program when invalid input is entered.
Why do you think we made the .all & .find methods class methods? Why not instance methods? They are class methods so that we can get information about all the customers without having to create an instance.
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 because one customer can have many orders, but an order can only have one customer. The Solar System project was also a one-to-many relationship because one solar system is made up of 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? The relationship is tracked in the CSV file by the Order CSV having the customer id. In my program, the Order has an instance of Customer which has all the information about that customer.
Did the presence of automated tests change the way you thought about the problem? How? Yes, I read the tests to better understand the problem.

@kaidamasaki
Copy link

Good job! You had some style issues with your code but otherwise everything was very solid and clear.

end


def self.find (id)

Choose a reason for hiding this comment

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

You shouldn't have spaces between a method name and its argument list.

Suggested change
def self.find (id)
def self.find(id)

That's where this warning is coming from:

/Users/kaida/Developer/cohort-12/grocery-store/lib/customer.rb:25: warning: parentheses after method name is interpreted as an argument list, not a decomposed argument


fulfillment_statuses = [:pending, :paid, :processing, :shipped, :complete ]
if !fulfillment_statuses.include?(@fulfillment_status)
raise ArgumentError

Choose a reason for hiding this comment

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

It would be helpful to include a message here so the user knows what went wrong.

Suggested change
raise ArgumentError
raise ArgumentError.new("Invalid fulfillment status: #{@fulfillment_status}")

data = CSV.read('data/orders.csv')
new_data = data.map do |person|
products = {}
person[1].split(';').each do |hash|

Choose a reason for hiding this comment

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

It's confusing to name a string hash, it would be clearer to do something like product or raw_product.


def self.find (id)
data = self.all
person_found = data.select {|person| person.id == id }

Choose a reason for hiding this comment

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

You may want to look at Enumerable#find.

# TODO: Your test code here!
end

it "Returns accurate information about the first order" do

Choose a reason for hiding this comment

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

Your indentation is kind of weird here and is why Github thought you deleted this test. (It looks like you moved some tests outside of describes.)

Some sort of formatter for VS Code could help you with noticing changes like this.

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