Skip to content

Grocery Store Project#37

Open
amanzan1 wants to merge 4 commits into
Ada-C9:masterfrom
amanzan1:master
Open

Grocery Store Project#37
amanzan1 wants to merge 4 commits into
Ada-C9:masterfrom
amanzan1:master

Conversation

@amanzan1

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? We want to group related classes and methods together.
What is accomplished with raise ArgumentError? The raise argument error lets the user know that what they are inputting into the computer is not defined and is not applicable to the program.
Why do you think we made the .all & .find methods class methods? Why not instance methods?
Why does it make sense to use inheritance for the online order? It makes sense to use inheritance for the online order because mostly all of the information is in the order class.
Did the presence of automated tests change the way you thought about the problem? How? Yes, because it made me question the way I was writing the code, especially by every little project requirement.

@CheezItMan

Copy link
Copy Markdown

Grocery Store

What We're Looking For

Feature Feedback
Baseline
Answered comprehension questions Check, module also namespace your code to prevent conflicting names.
Used Git Regularly You need to commit a lot more often. Also focus on functionality added, not "waves"
Wave 1
All provided tests pass Check
Using the appropriate attr_ for instance variables Check
Wave 2
All stubbed tests are implemented fully and pass MISSING You didn't write any tests here!
Appropriately parses the product data from CSV file in Order.all Check
Used CSV library only in Order.all (not in Order.find) Check
Used Order.all to get order list in Order.find Check
Wave 3
All stubbed tests are implemented fully and pass MISSING
Used inheritance in the initialize for online order Check
Used inheritance for the total method in online order Check
Use CSV library only in OnlineOrder.all Check
Used all to get order list in find Check, but see my note
Appropriately searches for Customer orders in find_by_customer You have a bug here, misunderstanding the requirements.
Additional Notes It's very disappointing that you didn't do any testing here. Did you not commit your testing files? You have quite good Order and OnlineOrder code here, but the lack of tests means this submission is not up to standard.

Comment thread lib/hello.rb Outdated
@@ -0,0 +1,2 @@
name = ARGV.first || "World"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This file should not be in your submission.

Comment thread lib/online_order.rb
class OnlineOrder < Order
attr_reader :customer_id, :status

def initialize (id, products, customer_id, status)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Very good

Comment thread lib/online_order.rb
return 10 + super
end

def self.find(id)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Since self.find really doesn't change between this class and the superclass Order you could just not override the find method and use Order's version.

Comment thread lib/online_order.rb

def self.find(id)
online_orders = OnlineOrder.all
online_orders.each do |o|

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Don't use o for a variable name.

Comment thread lib/online_order.rb
return nil
end

def self.all

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nicely done

Comment thread lib/order.rb


def self.all
require 'csv'

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The require 'csv' should go at the top of the file.

Comment thread lib/order.rb
# csv = CSV.open("../support/online_orders.csv")r
csv = CSV.open("support/orders.csv")
orders = []
csv.each do |l|

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

l is a bad variable name here!

Comment thread lib/order.rb
#puts l[1].split(';')

product_string = l[1]
product_smash = product_string.split(';')

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

product_smash????

Comment thread lib/order.rb

def self.find(id)
orders = Order.all
orders.each do |o|

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

variable names...

Comment thread lib/practice.rb
@@ -0,0 +1,4 @@
class EBook < Book

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This should not be in your submission.

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