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 C17-Marlyn Lopez #89

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

Conversation

marlopez24
Copy link

No description provided.

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 Marlyn! 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! :)

return False
else:
self.add(new_vendor.inventory[0])

Choose a reason for hiding this comment

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

Small note - you could reuse the swap_items() method here to DRY your code

for item in self.inventory:
if item.category == best_category:
if item.condition > highest_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 ):
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 objects so it's safe to set an empty string as a default value in your parameters :)

@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. :)

def __init__(self, category = "Clothing", condition = 0.0):
self.category = category
self.condition = float(condition)

Choose a reason for hiding this comment

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

This would be a great place to use super()! Rather than doing the work of binding these arguments to the instance attributes ourself, we can just pass the arguments to the parent class's constructor and have it do the work for us. So line 5 and 6 could be replaced with:
super().__init__(category = "Clothing", condition = condition)


class Clothing(Item):
def __init__(self, category = "Clothing", condition = 0.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"

from swap_meet.item import Item

class Decor(Item):

Choose a reason for hiding this comment

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

notes from clothing.py apply here too

pass
from swap_meet.item import Item

class Electronics(Item):

Choose a reason for hiding this comment

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

notes from clothing.py apply 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