Maja Octos Grocery_Store#38
Conversation
…ed. Updated assertions.
…nil if status is processing shipped or complete
…tances - not enough time to rewrite tests
|
I wasn't sure how the self.all and self.find methods were supposed to interact. Should the self.find method traverse the CSV file itself instead of the array created by self.all? |
Grocery StoreWhat We're Looking For
Great job overall! This code is clean and well-organized. I've left some inline comments below for you to review, but in general I am happy wth this submission. Regarding tests, your tests on this project look good to me, but if you want a little more guidance, remember that Arrange, Act, Assert pattern we talked about in class.
Remember also that you don't need to nail all your test cases the first time - every time you fix a bug or stumble over some bad behavior is an opportunity to write another test. This is certainly a skill that takes practice to get comfortable with. Keep up the hard work! |
| return order | ||
| end | ||
| end | ||
| return "ERROR: order does not exist" |
There was a problem hiding this comment.
Is it really an error if the order doesn't exist? A more friendly strategy might be to return nil, and let whoever called the method check for that value.
| def self.find(id) | ||
| # finds an order in the collection of Order instances | ||
| @@orders.each do |order| | ||
| if order.id == id |
There was a problem hiding this comment.
Referencing @@orders here means that .all must be called before .find. This is a dependency that we can avoid!
Instead of saying @@orders.each do, you could say self.all.each do. This will call the all method, which will read the orders from the file if that hasn't happened yet.
This is an example of what Metz is talking about in chapter 2 when she says "depend on behavior, not data".
|
|
||
| semicolon_split.each do |string| | ||
| key_value_split = string.split(':') | ||
| products[key_value_split[0]] = key_value_split[1].to_f |
There was a problem hiding this comment.
Good work getting this tricky logic sorted out.
| attr_reader :id, :products | ||
|
|
||
| FILE_NAME = "support/orders.csv" | ||
|
|
| describe "Order Wave 2" do | ||
| before do | ||
| @all_orders = Grocery::Order.all | ||
| # @orders = Grocery::Order |
There was a problem hiding this comment.
Calling Order.all here masks that dependency between .find and .all that I discussed above.
| # TODO: Your test code here! | ||
| @all_online_orders.each do |order| | ||
| order.must_be_kind_of Grocery::Order | ||
| end |
There was a problem hiding this comment.
Shouldn't this be order.must_be_kind_of Grocery::OnlineOrder?
The reason this test passes is because an OnlineOrder is a kind of Order.
|
|
||
| def initialize(id, products, customer_id, status = :pending) | ||
| super(id, products) | ||
| @customer_id = customer_id |
| return nil | ||
| else | ||
| @products[product_name] = product_price | ||
| end |
There was a problem hiding this comment.
This misses the behavior we had with Order, where if the product was already in the order the method would return false. One way to get that behavior would be to call super here.
| customer_orders << order | ||
| end | ||
| end | ||
| return customer_orders |
There was a problem hiding this comment.
I like that this will return an empty array if no orders match.
2 participants
Grocery Store
Congratulations! You're submitting your assignment.
Comprehension Questions
raise ArgumentError?.all&.findmethods class methods? Why not instance methods?