Emilce -- Grocery Store -- Octos #32
Conversation
|
I would like to continue working on Wave 3. |
emilcecarlisa
changed the title
Grocery StoreWhat We're Looking For
This is a good start, but there are definitely some pieces missing from this submission, and it also seems like there may have been some requirements misses. One thing that might help going forward is to focus more on keeping yourself organized and making sure you've got a process to follow. An example of this process for a method might look something like this:
There is a lot of good work that went into this project, and I want to acknowledge the hours you've been putting in as well. Hopefully following a process like this will help to make your work more efficient. Keep that level of dedication and keep your spirits up, and things will definitely come together. |
| require "awesome_print" | ||
|
|
||
| ORDERS_CSV = "../support/orders.csv" # take off ../ if running rake | ||
|
|
There was a problem hiding this comment.
Instead of changing the path, you can run the ruby file from the project root:
$ ls
README.md Rakefile feedback.md lib specs support
$ ruby lib/order.rb
[ ... program output ...]| class Order | ||
| attr_reader :all, :total | ||
| attr_accessor :id, :products, :add_product | ||
|
|
There was a problem hiding this comment.
I'm curious why you chose to make these attr_accessors. As far as I can tell, you don't set these values anywhere outside this class, nor should you given the spec.
Leaving it an attr_reader is a safety measure, so that if you accidentally do something like if order.id = 3 (using = instead of ==), ruby will give you a big error instead of doing the wrong thing.
Also, add_product is an instance method, but attr_accessors only work for instance variables. Saying attr_accessor :add_product doesn't make any sense.
| products_string.each do |product| # "cucumber:5" | ||
| key = product.split(":")[0] # ["cucumber", "5"] | ||
| value = product.split(":")[1] | ||
| products_hash[key] = value.to_f # or # products_hash.push(key: value) |
There was a problem hiding this comment.
Good work getting this tricky logic sorted out.
| unless order.id == id | ||
| raise ArgumentError.new("Order number could not be found") | ||
| end | ||
| return order.products |
There was a problem hiding this comment.
This isn't quite what we were looking for with this method. What we want is for you to, given an ID, find the entire Order, not the list of products.
Moreover, putting the raise inside the loop like this means that as soon as you look at an order that isn't the one you want, you'll throw an error. Instead, you should do something like this:
def self.find(id)
self.all.each do |order|
if order.id == id
return order
end
end
raise ArgumentError.new("Order number could not be found")
endWhat this code says is, "if I looked at every single order and still couldn't find the one I wanted, then I'll raise an error".
| return false | ||
| else | ||
| @products.store(product_name, product_price) | ||
| return true |
There was a problem hiding this comment.
On line 71, it would be a little more clear to say @products[product_name] = product_price. This square brackets notation makes it clear that you're adding a key/value pair to the hash, whereas calling a method means the reader has to think about what's going on.
| order_array.each do |order| | ||
| if order.id == 1 | ||
| order.products.must_equal({"Slivered Almonds"=>22.88, "Wholewheat flour"=>1.93, "Grape Seed Oil"=>74.9}) | ||
| end |
There was a problem hiding this comment.
Since these tests are for Order.find, you should be calling Order.find here. Remember, the Act step of Arrange, Act, Assert is always to call the method you're testing.
|
|
||
| class OnlineOrder < Grocery::Order | ||
| # OnlineOrder is an Order | ||
| # customer object |
There was a problem hiding this comment.
This class should be inside the Grocery module.
|
|
||
| def initialize(id, products, cust_id, status) | ||
| @id = id | ||
| @products = products |
There was a problem hiding this comment.
Instead of saving @id and @products yourself here, you should call super(id, products) and have Order's initialize method do the work for you.
| if @products.key? product_name | ||
| puts "This is already included in the order." | ||
| return false | ||
| else |
There was a problem hiding this comment.
Instead of copying this code from Order, you should use super(product_name, product_price) to access the existing behavior.
|
|
||
| def add_product(product_name, product_price, cust_id, status) | ||
| if status != :pending || status != :paid | ||
| raise ArgumentError.new("Can only add new product if your status is pending or paid") |
There was a problem hiding this comment.
This method should not take a customer ID or a status. The method signature should be the same as for Order.
2 participants
Grocery Store
Congratulations! You're submitting your assignment.
Comprehension Questions
raise ArgumentError?raise-ing an exception is like saying "something's wrong", and will cause the program to exit. If you're calling a method that you know might raise an exception, and you know what to do in that situation, you canrescuethe error, which will prevent the program from exiting..all&.findmethods class methods? Why not instance methods?Orderto 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 anOrder, and then you can callOrder.allto get the list.