Skip to content

Conversation

@Oh-KPond
Copy link

@Oh-KPond Oh-KPond commented Feb 20, 2018

Grocery Store

Congratulations! You're submitting your assignment.

Comprehension Questions

Question Response
Why is it useful to put classes inside modules? By putting classes inside modules, the program can run different methods without causing collision problems. E.g. Grocery::Customer.id and Grocery::OnlineOrder.id both call an instance variable @id but with namespacing, no naming collisions happen
What is accomplished with raise ArgumentError? raise ArgumentError creates a better user experience. So when an invalid entry is given, the user is alerted instead of given nothing or an error they don't understand.
Why do you think we made the .all & .find methods class methods? Why not instance methods? By making the .all & .find methods class methods we are able to use them for all instances instead on just one instance.
Why does it make sense to use inheritance for the online order? Since online order is a type of order, it's nice to be able to use some of the methods from the order class instead of duplicating code.
Did the presence of automated tests change the way you thought about the problem? How? I love testing! I can't wait to get better at creating them. While I was creating tests I was able to think about all the ways that I might be able to get to the solution. Writing tests really helped me process what the program was looking for. Like I said though, I'm really looking forward to getting better at test writing.

@Oh-KPond
Copy link
Author

Oh-KPond commented Feb 20, 2018

Hahaha... okay so maybe I have an excessive amount of commits. I committed whenever I thought that I had finished a thought. I will attempt to bundle better. In the future, I may commit tests and code together instead of the test, then the code, then all the refactoring. :shrug #design

p.s. I know that some of my tests are still failing... darn!!

@CheezItMan
Copy link

Grocery Store

What We're Looking For

Feature Feedback
Baseline
Answered comprehension questions Check,
Used Git Regularly Lots of commits, that's good. You also mostly have good commit messages, focus on the functionality added or changes made, not on waves.
Wave 1
All provided tests pass Check
Using the appropriate attr_ for instance variables Your products doesn't need to be an accessor.
Wave 2
All stubbed tests are implemented fully and pass Check
Appropriately parses the product data from CSV file in Order.all Check
Used CSV library only in Order.all (not in Order.find) Check
Used Order.all to get order list in Order.find Check
Wave 3
All stubbed tests are implemented fully and pass Not quite, see my in-line notes.
Used inheritance in the initialize for online order Check my note on your attr_reader
Used inheritance for the total method in online order Check, well done
Use CSV library only in OnlineOrder.all Check
Used all to get order list in find Check, good job not overriding the existing method from Order.
Appropriately searches for Customer orders in find_by_customer NO There is a bug here. It always raises an error.
Additional Notes Overall pretty good. You got most of the primary requirements and a few optionals. Nice work!

all_customers.each do |customer|
if id == customer.id
return customer
elsif id == "first"

Choose a reason for hiding this comment

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

Not really in the requirements, but interesting.

Copy link
Author

Choose a reason for hiding this comment

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

hahaha thanks!

lib/order.rb Outdated
class Order
attr_reader :id, :products
attr_reader :id
attr_accessor :products

Choose a reason for hiding this comment

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

This does not need to be an attr_accessor

# act
Grocery::Order.all
# assert
Grocery::Order.all.must_be_kind_of Array

Choose a reason for hiding this comment

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

You should also check that every element is an Order

# act
Grocery::Order.all[-1].id
# assert
Grocery::Order.all[-1].id.must_equal CSV.read('support/orders.csv', 'r')[-1][0]

Choose a reason for hiding this comment

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

Clever

#arrange
# N/A
# act
Grocery::Order.find("first")

Choose a reason for hiding this comment

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

It would be better to find the first Order of the file by the ID field of that order.

Something like: Grocery::Order.find(1)


module Grocery
class OnlineOrder < Order
attr_reader :id, :customer, :products, :status

Choose a reason for hiding this comment

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

Since this class inherits from Order, you don't need to define an attr_reader for id and products because they inherit them from Order.


#self.find should come from Grocery::Order

def self.find_by_customer(customer_id)

Choose a reason for hiding this comment

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

This method always raises the ArgumentError, you should return order_array (empty if no order has that customer or if the customer has orders it should return an array of those orders.

# act
Grocery::OnlineOrder.all
# assert
Grocery::OnlineOrder.all.must_be_kind_of Array

Choose a reason for hiding this comment

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

Again make sure all the elements are OnlineOrder objects

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