Skip to content

Conversation

@AngelaPoland
Copy link

@AngelaPoland AngelaPoland 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? Modules group together related ideas and organize code with related classes. It also helps avoid naming collisions
What is accomplished with raise ArgumentError? They are used in methods to notify users of your class, if you think certain kinds of arguments/parameters aren’t acceptable.
Why do you think we made the .all & .find methods class methods? Why not instance methods? We needed to search through the whole CSV files and every instance of the class not just one instance of the class.
Why does it make sense to use inheritance for the online order? It makes sense because an Online Order is still a type of order and thus should inherit the same kinds of methods as it’s “parent” class in order to reduce redundancies.
Did the presence of automated tests change the way you thought about the problem? How? Yes. Honestly, I had a lot of difficulty with the tests. In theory, I understand that they help you debug your code even before writing it which seems so cool. In practice though, I got extremely stuck and kept getting errors, which meant I wasn’t writing the tests correctly, and, especially for Wave 3, I just could not get past that those error messages. I ended up just writing the code without the tests after a while. I’d really like more time to review what certain error messages mean. I got through the wave 2 tests with help from my sister’s boyfriend who is a test fanatic, but once left alone I couldn’t get it. For wave 3 I only got the very first test to even get to fail (and eventually did get it to pass) but I ran out of time to complete the assignment in full. Due to this, I’m also sure I have errors in my code for the Online Order class (specifically the find_by_customer method.
I hope to get more feedback and possibly further tutoring on tests, class inheritance, and class methods.

…hen I put puts statments in the if/else loop its in...both are printing out and I don't know why. (even though the test technically passes in specs test
…t updated the first wave 1 test for it though.
… did in order specs that are easier to adjust for online-orders.
@CheezItMan
Copy link

Grocery Store

What We're Looking For

Feature Feedback
Baseline
Answered comprehension questions Check
Used Git Regularly Pretty good number of commits, but focus on functionality added rather than talk of waves.
Wave 1
All provided tests pass For wave 1 yes, see my notes about Order.
Using the appropriate attr_ for instance variables For wave 1, yes
Wave 2
All stubbed tests are implemented fully and pass See my notes for things not tested.
Appropriately parses the product data from CSV file in Order.all Check, well done here!
Used CSV library only in Order.all (not in Order.find) Check, well done
Used Order.all to get order list in Order.find Check
Wave 3
All stubbed tests are implemented fully and pass Mostly, I left notes in your code.
Used inheritance in the initialize for online order Yes, but you didn't take advantage of some of the features of the parent class. I left notes in your code.
Used inheritance for the total method in online order Check, see the add_product method for some things you missed there.
Use CSV library only in OnlineOrder.all Check
Used all to get order list in find Check and took advantage of inheritance here! Great
Appropriately searches for Customer orders in find_by_customer Check, but I left notes about a bug in it.
Additional Notes Overall well done, you hit most of the learning goals and showed progress in testing. I don't think you'll be surprised to hear that you need more practice in testing, but this was pretty good. Read my inline notes and ask me any questions you have. See if you can focus on being through in testing for Scrabble. That will be good practice.


class OnlineOrder < Order

attr_reader :id, :products, :customer_id, :fulfillment_status

Choose a reason for hiding this comment

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

You can inherit the helper methods for id and products and just have:

attr_reader :customer_id, :fulfillment_status

attr_reader :id, :products, :customer_id, :fulfillment_status

def initialize(id, products, customer_id, fulfillment_status)
@id = id

Choose a reason for hiding this comment

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

You should have:

super(id, products)

here to initialize @id and @products

# tip from Katie: 16 and 22 are the customer id's that exsit but do not have orders
end

def self.all

Choose a reason for hiding this comment

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

Nicely done!

def self.find_by_customer(customer_id)

OnlineOrder.all.each do |single_order|
order_list = []

Choose a reason for hiding this comment

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

You should probably have order_list = [] outside the loop.

end
end

def total

Choose a reason for hiding this comment

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

Great!

# TODO: Your test code here!
end
xdescribe "OnlineOrder.find_by_customer" do
xit "Returns an array of online orders for a specific customer ID" do

Choose a reason for hiding this comment

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

This should return an array, so:

Grocery::OnlineOrder.find(1).must_be_instance_of Array
Grocery::OnlineOrder.find(1).each do |element|
  element.must_be_instance_of Grocery::OnlineOrder
end

# Act
order.add_product("salad", 4.25)

# Assert

Choose a reason for hiding this comment

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

Smart to put these comments in to see the steps.

all_orders.class.must_equal Array

all_orders.each do |order|
order.must_be_instance_of Grocery::Order

Choose a reason for hiding this comment

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

Good!

describe "Order.find" do

before do
@order_class = Grocery::Order

Choose a reason for hiding this comment

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

Not sure what the point of this is, just to save typing?

it "Can find the first order from the CSV" do
# TODO: Your test code here!

@order_class.find(1).must_be_instance_of Grocery::Order

Choose a reason for hiding this comment

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

You should also check to ensure the values of the found Order have the right values, check the id, products etc.

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