Skip to content

Ampers Mariko A. Grocery Store#26

Open
marikoja wants to merge 6 commits into
Ada-C9:masterfrom
marikoja:master
Open

Ampers Mariko A. Grocery Store#26
marikoja wants to merge 6 commits into
Ada-C9:masterfrom
marikoja:master

Conversation

@marikoja

Copy link
Copy Markdown

Grocery Store

Congratulations! You're submitting your assignment.

Comprehension Questions

Question Response
Why is it useful to put classes inside modules? Modules are our namespaces which help us avoid naming conflicts
What is accomplished with raise ArgumentError? We raised an error to indicate to the user that they had entered an invalid user ID, the customization allows us to make program specific errors
Why do you think we made the .all & .find methods class methods? Why not instance methods? We used Class methods for all and find because we wanted to search through all the instances of our data rather than a single instance. Instance methods are for a single instance of the class.
Why does it make sense to use inheritance for the online order? We want online orders to have the same properties as order with some further modifications so it makes sense to use inheritance.
Did the presence of automated tests change the way you thought about the problem? How? Using the tests was hard to process but also forced me to think about how I would be using the code and attempt to account for potential issues.

@tildeee

tildeee commented Feb 26, 2018

Copy link
Copy Markdown

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 x
Wave 3
All stubbed tests are implemented fully and pass x
Used inheritance in the initialize for online order x
Used inheritance for the total method in online order weird implementation of total, also used the alias keyword
Use CSV library only in OnlineOrder.all x
Used all to get order list in find x
Appropriately searches for Customer orders in find_by_customer x, n/a
Additional Notes

Overall the code looks good. I think the implementation looks good and the tests are readable and accurate and thorough.

In every other test you wrote, you did a great job of hardcoding the expected value and the tests were overall great. There is one test though that isn't written so well in OnlineOrder spec:

describe "#total" do
    it "Adds a shipping fee" do
      @new_order.total.must_equal (@new_order.order_total + 10)
    end
...

You're testing if the method total on the instance @new_order is equal to another property (.order_total) on the same instance @new_order...

Also for checking float value equality, keep in mind that you might want to use must_be_within_delta instead of must_equal.

I would discourage use of the alias keyword!

I would encourage you to review super if inheritance and invoking super isn't clear yet!

Overall this meets all of the requirements and the project looks good. Good work!

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