Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Leaves - Cloudy #49

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Leaves - Cloudy #49

wants to merge 4 commits into from

Conversation

OhCloud
Copy link

@OhCloud OhCloud commented Aug 26, 2019

Grocery Store

Congratulations! You're submitting your assignment.

Comprehension Questions

Question Response
What is accomplished with raise ArgumentError? catches edge cases
Why do you think we made the .all & .find methods class methods? Why not instance methods? we made them class methods because were calling it back on itself.
Think about the relation between Order and Customer. Is this relation one-to-one, one-to-many, or something else? How does that compare to the Solar System project? sometimes its one to one and sometimes its one to many. for order, it couldve gone either way with the amount of orders, and for customer, it was one to one
How is the relation between Order and Customer tracked in the CSV file? How is it tracked in your program? Why might these be different? we require csv, customer, order so files talk to each other and its tracked in the program because theyre treated as instances
Did the presence of automated tests change the way you thought about the problem? How? yes, the requirements vs the suggestions can be confusing but otherwise, usually it aids as aguide

@dHelmgren
Copy link

Grocery Store

What We're Looking For

Feature Feedback
Baseline
Answered comprehension questions Quick misnomer: We aren't 'calling class methods back on [the class]' .find and .all just don't make sense to call on a single instance.
Used Git Regularly I'd like to see more regular commits on a project of this size.
Wave 1
All provided tests pass Yes
Using the appropriate attr_ for instance variables yes
Wave 2
All stubbed tests are implemented fully and pass no, not all tests pass, and a few of them are not implemented.
Used CSV library only in .all (not in .find) yes
Appropriately parses the product data from CSV file in Order.all no, see comment
Order.all calls Customer.find to set up the composition relation no
Additional Notes We should talk about this. It feels like you ran out of time on this, but I didn't hear from you before it was turned in. Remember, I want code that is Working, Complete, and Tidy in that order.

describe "Order.all" do
it "Returns an array of all orders" do
# TODO: Your test code here!
end

it "Returns accurate information about the first order" do

Choose a reason for hiding this comment

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

This test fails, see comment in orders.rb

# Check that all data was loaded as expected
expect(order.id).must_equal id
expect(order.products).must_equal products
expect(order.customer).must_be_kind_of Customer
expect(order.customer.id).must_equal customer_id
expect(order.fulfillment_status).must_equal fulfillment_status
end

it "Returns accurate information about the last order" do
# TODO: Your test code here!

Choose a reason for hiding this comment

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

It's a good idea to scan your code for TODO:s before submitting.

}
customer_id = order[2]
fulfillment_status = order[3]
order = Order.new(order_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.

As written, this code will always fail. initialize is expecting a :symbol for fulfillment_status (see lines 13-15). Your test caught this, but it looks like it didn't get fixed.

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