Skip to content

Conversation

@SelamawitA
Copy link

@SelamawitA SelamawitA commented Feb 22, 2018

Grocery Store

Congratulations! You're submitting your assignment.

Comprehension Questions

Question Response
Why is it useful to put classes inside modules? For organization of objects with similar functionality or to prevent naming errors in classes with the same name but different behavior.
What is accomplished with raise ArgumentError? Raising an ArgumentError gives a programmer the ability to see if a failure has occurred without a program breaking or continuing with a condition being true. An ArgumentError can be paired with a rescue to trap a user into providing correct information to ensure that a program runs as the design intended.
Why do you think we made the .all & .find methods class methods? Why not instance methods? The information (CSV data) is consistent among all instants of the class. An instance method would have worked as well but the CSV data would have had to be passed in with each instance of the object or a constant would need to be declared at the beginning of the class.
Why does it make sense to use inheritance for the online order? Online order uses a lot of the same methods, variables and has a relationship to order that makes sense for a IS-A.
Did the presence of automated tests change the way you thought about the problem? How? I did a lot more refactoring in the middle of my programming rather than waiting towards the end since I was forced to think about how it could fail/break before completing a class.

…cery order including tax, and allow user to add a product with boolean to confirm addition
…es and an array of store ID, then stores into an array of objects. Included self.find() - checks user input for store ID and returns associated object or NIL.
…clude shipping fee, overrode add_product to raise argument error, added self.all to read in from CSV, and self.find to find object by ID.
@tildeee
Copy link

tildeee commented Feb 25, 2018

Grocery Store

What We're Looking For

Feature Feedback
Baseline
Answered comprehension questions x
Used Git Regularly x
Wave 1
All provided tests pass x
Using the appropriate attr_ for instance variables x
Wave 2
All stubbed tests are implemented fully and pass x
Appropriately parses the product data from CSV file in Order.all x
Used CSV library only in Order.all (not in Order.find) x
Used Order.all to get order list in Order.find populated @@all_orders in Order.all. It would still be better to explicitly call Order.all in the Order.find method because... what if the program hadn't populated @@all_orders before calling .find? Then @@all_orders would be empty!
Wave 3
All stubbed tests are implemented fully and pass Out of the box, I get an error for OnlineOrder::OnlineOrder.find#test_0001_Will find an online order from the CSV
Used inheritance in the initialize for online order x
Used inheritance for the total method in online order didn't use super/the implementation of total in Order
Use CSV library only in OnlineOrder.all x
Used all to get order list in find Same comment as above: populated @@online_orders in Order.all. It would still be better to explicitly call OnlineOrder.all in the OnlineOrder.find method because... what if the program hadn't populated @@online_orders before calling .find? Then @@online_orders would be empty!
Appropriately searches for Customer orders in find_by_customer x
Additional Notes

Selam!!!!! Your indentation!!!!! Is off in so many places!!!!!!! Selam!!!!!! Please take care to adjust your indentation as you work

You have one test that's red: OnlineOrder::OnlineOrder.find#test_0001_Will find an online order from the CSV
I'm adding a comment to the code about this.

Your class is named Onlineorder and should be named OnlineOrder and also be in the Grocery module

You used class variables in your solution--
When using a class variable, you can optionally declare the variable and give them an initial value. To do so, you put them within the class definition of the class they apply to, but not in any method... like right under attr_ methods and before def initialize would be good. Right now you put @@all_orders outside of any class or module so it's kind of just... floating and not doing anything

Good job with Customer, too

With regards to your tests: I think you did a good job of writing them! They're pretty clear and good. A few of them could be more specific; for instance, your .all methods could make sure that not only the value is an instance of Array (which you did accurately), but also that each element in the Array was an instance of Order or OnlineOrder.

Also, a lot of the tests were missing and you didn't do them. They were especially the kinds of tests that were a little bit more deep and you had to check data more deeply. I think you need practice writing tests that are more deep than just checking to make sure IDs exist, etc, so I look forward to the future where you'll submit some good tests

In general, good job; half of the OnlineOrder tests are missing and I think there are some detailed things about your code to work on, but overall you hit some of the requirements well

# TODO: Your test code here!
online_orders = Onlineorder.all
searched = online_orders.find('45')
searched.id.must_equal '45'
Copy link

Choose a reason for hiding this comment

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

You get an error when running this test-- this test isn't written correctly. Why though?

the error is searched doesn't have a method .id. What is the value of searched?

The value of searched isn't an instance of OnlineOrder, which is what we expected. It's an instance of Enumerator. Why?

You set searched to the value of online_orders.find('45'). online_orders is an array of OnlineOrder instances. Therefore, Ruby calls on Array's implementation of .find.

If you want to use the .find method you defined as a class method off of OnlineOrder, you're going to need to call it with the correct syntax: Grocery::OnlineOrder.find('45')

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