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

Paul Knapp #11

Open
wants to merge 35 commits into
base: main
Choose a base branch
from
Open

Paul Knapp #11

wants to merge 35 commits into from

Conversation

Paul-Knapp
Copy link

two methods for iteration 3 + passes all written tests


def initialize(info)
@name = info[:name]
@price = info[:price]
Copy link
Contributor

Choose a reason for hiding this comment

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

The interaction pattern asks to clean the string (remove the dollar sign) and convert to a float!

Copy link
Contributor

Choose a reason for hiding this comment

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

Great commit history - nice stuff Paul!

@@ -0,0 +1,2 @@
--format documentation
--color
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

end

def sorted_item_list
vendors.flat_map { |vendor| vendor.inventory.keys.map(&:name)}.uniq.sort
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice one liner!

def total_inventory
all_inventory = {}

@vendors.each do |vendor|
Copy link
Contributor

Choose a reason for hiding this comment

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

Theres probably an easier way to do this. Ask perplexity for some refactoring tips!


def potential_revenue
@inventory.sum do |item,quantity|
item.price.gsub('$', '').to_f * quantity
Copy link
Contributor

Choose a reason for hiding this comment

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

Youre cleaning the string here!! Nice. Do it on the item instantiation, though.

end

def stock(item, quantity)
@inventory[item] = (@inventory[item] || 0 )+ quantity
Copy link
Contributor

Choose a reason for hiding this comment

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

OO I like this

Comment on lines +14 to +24
it '#exists' do
expect(@market).to be_an_instance_of(Market)
end

it '#has a name' do
expect(@market.name).to eq ("South Pearl Street Farmers Market")
end

it '#has no vendors' do
expect(@market.vendors).to eq([])
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Functionality that happens in the instantiation can be consolidated into a single test, if you'd like to save time and lines.

end

it '#can show vendors that sell an item' do
@vendor1.stock(@item1, 35)
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider testing stock in isolation. This is a nice test though, I have no problem with what you did here.

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