Skip to content

Conversation

@kcforsman
Copy link

Grocery Store

Congratulations! You're submitting your assignment.

Comprehension Questions

Question Response
Why is it useful to put classes inside modules? It allows you to utilize namespace. If two different classes that were needed in a a program had the same name for a method, you can distinguish them by module name.
What is accomplished with raise ArgumentError? From what I could tell, it prevents the program from move forward and running the code while also returning an error that can include a message that clarifies what was done wrong.
Why do you think we made the .all & .find methods class methods? Why not instance methods? Because we needed access to all the instances to find specific details. If they were stored as instances alone, the information wouldn't be stored within the class itself.
Why does it make sense to use inheritance for the online order? Online orders can be considered a type of order. As such, they share many methods that are only slightly different.
Did the presence of automated tests change the way you thought about the problem? How? I think I was more focused on how to successfully write the test. I will say that thinking of the tests helped me with breaking down the problems into smaller parts.

@CheezItMan
Copy link

Grocery Store

What We're Looking For

Feature Feedback
Baseline
Answered comprehension questions Check
Used Git Regularly Good number of commits, try to focus on functionality and not on waves for commit messages
Wave 1
All provided tests pass Check
Using the appropriate attr_ for instance variables Check
Wave 2
All stubbed tests are implemented fully and pass Check
Appropriately parses the product data from CSV file in Order.all
Used CSV library only in Order.all (not in Order.find) Check, see my in-line notes
Used Order.all to get order list in Order.find Check
Wave 3
All stubbed tests are implemented fully and pass Check
Used inheritance in the initialize for online order Check
Used inheritance for the total method in online order Check
Use CSV library only in OnlineOrder.all Check
Used all to get order list in find Nice work not overriding the find method.
Appropriately searches for Customer orders in find_by_customer Yes, but you have one unneeded line.
Additional Notes Very well done, I had some small notes, but very nice!

end
# returns array of all orders in the CSV
def self.all
@@all_orders = []

Choose a reason for hiding this comment

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

If you're resetting @@all_orders each time self.all is called, why make it a class variable. It's just as good as a local variable.

return order
end
end
raise ArgumentError

Choose a reason for hiding this comment

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

You should probably not raise an ArgumentError here. It would make sense to return nil when you can't find such an order.

super(id, products)
@customer_id = customer_id
@customer = Customer.find(@customer_id)
if status.class == Symbol

Choose a reason for hiding this comment

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

This if/else is unnecessary as symbols also have a to_sym method

# inherited method from order class that adds 10 for shipping to the total
# if order is greater than 0
def total
if super == 0

Choose a reason for hiding this comment

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

well done!

# method to return an array of all orders from a single customers
# from the input of their customer id
def self.find_by_customer(customer_id)
Customer.find(customer_id)

Choose a reason for hiding this comment

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

Why have this line?

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