Skip to content

Conversation

@diklaaharoni
Copy link

@diklaaharoni diklaaharoni commented Feb 20, 2018

Grocery Store

Congratulations! You're submitting your assignment.

Comprehension Questions

Question Response Instructor Feedback
Why is it useful to put classes inside modules? because it is a good way to group together similar things
What is accomplished with raise ArgumentError? don't know This question is a holdover from previous cohorts when the requirements were different, so this is fine.
Why do you think we made the .all & .find methods class methods? Why not instance methods? don't know To think about this, consider how your code would be different if we had made them instance methods. In that case, you would need to already have an instance of Order to get the list of all orders, which doesn't make much sense. With class methods, all you need is to know that there is such a thing as an Order, and then you can call Order.all to get the list.
Why does it make sense to use inheritance for the online order? because online order have similar properties
Did the presence of automated tests change the way you thought about the problem? How? yes, it thought me to think about the way i write my code

@droberts-sea
Copy link

Grocery Store

What We're Looking For

Feature Feedback
Baseline
Answered comprehension questions yes - see my responses above
Used Git Regularly Your commit messages are good, but I would like to see you committing more often.
Wave 1
All provided tests pass yes
Using the appropriate attr_ for instance variables yes
Wave 2
All stubbed tests are implemented fully and pass yes
Appropriately parses the product data from CSV file in Order.all yes
Used CSV library only in Order.all (not in Order.find) yes
Used Order.all to get order list in Order.find yes
Wave 3
All stubbed tests are implemented fully and pass N/A
Used inheritance in the initialize for online order N/A
Used inheritance for the total method in online order N/A
Use CSV library only in OnlineOrder.all N/A
Used all to get order list in find N/A
Appropriately searches for Customer orders in find_by_customer N/A
Additional Notes

This is a good start. Your code for waves 1 and 2 is well-organized, accurate and easy to read, and I'm happy with your test cases for wave 2 as well.

I am concerned that you did not make it to wave 3. Not getting to practice inheritance will make next week's project more difficult, and it also indicates that you're writing code slower than we would like. Being able to quickly and confidently solve problems is something that will grow with time and practice. One way to drill it is to take a small practice problem and build it starting from an empty file. The loop practice problems or something like flower or library is a good option. This is also something you can target with a tutor.

This stuff isn't easy. I've seen you putting in the hours, and I really appreciate how hard you've been working. Keep up that dedication and keep your spirits high, and I know you'll get there.

products_hash[product_name] = product_price
end
order = Order.new(id, products_hash)
orders << order

Choose a reason for hiding this comment

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

Good work getting this complex bit of logic worked out!

order_id = nil
self.all.each do |order|
if order.id == id
order_id = order

Choose a reason for hiding this comment

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

I'm not sure the name order_id describes what you're using this variable for. It looks like it's storing an order, not an ID. Something like matching_order might be a better choice.



ap Grocery::Order.all
# puts " Product #{product[0]}: #{product[1]}"

Choose a reason for hiding this comment

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

Please remove debugging print statements like this before submitting projects.

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