Skip to content

Branches - Amal - #26

Open
ashassan wants to merge 11 commits into
Ada-C12:masterfrom
ashassan:master
Open

Branches - Amal #26
ashassan wants to merge 11 commits into
Ada-C12:masterfrom
ashassan:master

Conversation

@ashassan

Copy link
Copy Markdown

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
Copy Markdown

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

Comment thread lib/customer.rb
end


def self.find (id)

Copy link
Copy Markdown

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

Comment thread lib/order.rb

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

Copy link
Copy Markdown

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}")

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

Copy link
Copy Markdown

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.

Comment thread lib/customer.rb

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

Copy link
Copy Markdown

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.

Comment thread test/order_test.rb
# TODO: Your test code here!
end

it "Returns accurate information about the first order" do

Copy link
Copy Markdown

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