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

Otters - Tori Shade #101

Open
wants to merge 31 commits into
base: master
Choose a base branch
from
Open

Otters - Tori Shade #101

wants to merge 31 commits into from

Conversation

ToriShade
Copy link

No description provided.

ToriShade added 30 commits April 4, 2022 11:34
Copy link

@kendallatada kendallatada left a comment

Choose a reason for hiding this comment

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

Hi Tori! Your submission has been scored as green. Please check your code to see the comments I left. Let me know if you have any questions. Nice work! :)


def __init__(self, inventory = None):
'''Vendor class is instantiated'''

Choose a reason for hiding this comment

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

Beautiful docstrings 👏

return False
else:
return self.swap_items(other, self.inventory[0], other.inventory[0])

Choose a reason for hiding this comment

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

Nice usage of self.swap_items()

for item in self.inventory:
if item.category == category:
if item.condition > current_condition:

Choose a reason for hiding this comment

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

small note - you could combine these conditional statements on one line using the and keyword. So, if item.category == category and item.condition > condition:

def __init__(self, category=None, condition = 0, age = 0):
if not category:
category = ""

Choose a reason for hiding this comment

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

Strings are immutable so you can set an empty string as a default value in the parameters :)

return f"basic"
if self.condition == 5:
return f"vibe"

Choose a reason for hiding this comment

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

Lol @ "vibe"

small note - make sure to have a blank line at the end of all your Python files. It's convention.

@pytest.mark.skip
@pytest.mark.integration_test
#@pytest.mark.skip
#@pytest.mark.integration_test

Choose a reason for hiding this comment

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

In the future, don't worry about commenting out @pytest.mark.integration. This decorator just specifies that these tests are integration tests so they should run after all the unit tests have been executed. :)

'''Clothing class is instantiated and inherits from Item class'''

def __init__(self, category = "Clothing", condition = 0, age = 0):

Choose a reason for hiding this comment

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

small note - if all items for this child class should have their category attribute set to "Clothing", do we need to have category as a parameter in the constructor? This makes it possible for users to set the category to something else which could lead to unexpected problems in our application. Think about how you could update your code to make sure the category for this class is always set to "Clothing"

'''Decor class is instantiated and inherits from Item class'''

def __init__(self, category = "Decor", condition = 0, age = 0):

Choose a reason for hiding this comment

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

note about category parameter applies here too

class Electronics(Item):
'''Electronics class is instantiated and inherits from Item class'''

def __init__(self, category = "Electronics", condition = 0, age = 0):

Choose a reason for hiding this comment

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

note about category parameter applies here too

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