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

Oscar Submission #14

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

Oscar Submission #14

wants to merge 5 commits into from

Conversation

ossar3
Copy link

@ossar3 ossar3 commented Sep 16, 2024

No description provided.

def initialize (info)
@name = info[:name]
@price = info[:price]
#takes out all weird characters and symbols then convert to float
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice note here! You could have done the below method in the initialize function instead, but this works!

@market.add_vendor(@vendor1)
@vendor1.stock(@item1, 35)
@vendor1

Copy link
Contributor

Choose a reason for hiding this comment

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

You're missing an end here! Better indentation and conventions could have make this easy to see! Please continue to work on this!



def stock(item, amount)
if @inventory.include?(item)
Copy link
Contributor

Choose a reason for hiding this comment

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

Great method here! Checking it includes the item, and += the quantity

end

def price
@price.gsub(/[^\d.]/, '').to_f
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know what this regex is doing here! It works, but regex and can be confusing. Make sure you know whats going on with this code! You could have used the string method .delete("$") here to the same effect.



def add_vendor(vendor)

Copy link
Contributor

Choose a reason for hiding this comment

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

vendors << vendor would go here.

@market.add_vendor(@vendor2)
@market.add_vendor(@vendor3)
expect(@market.vendors).to eq [@vendor1, @vendor2, @vendor3]
expect(@market.vendor_names).to eq[]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good test! Just needed to finish the method that does the logic.

@@ -1,3 +1,30 @@
class Vendor
attr_reader :name

attr_accessor :inventory
Copy link
Contributor

Choose a reason for hiding this comment

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

This could probably exist as an attr_reader. You're not going to update this value directly outside of this class.

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