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

Whales- Sabrina H. #90

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

Conversation

sabrinahuwang
Copy link

No description provided.

Copy link

@alope107 alope107 left a comment

Choose a reason for hiding this comment

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

Awesome job Sabrina! I especially liked the way you handled the constructors of the subclasses. This project is definitely green!

Comment on lines +3 to +5
def __init__(self, condition=0):
super().__init__(category="Clothing", condition=condition)

Choose a reason for hiding this comment

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

Awesome use of super and very clever to see you didn't need category as an argument to this subclass constructor!

Comment on lines +9 to +26
if self.condition < 1:
return "BAD. Buy at your own risk"

elif self.condition < 2:
return "Not the worst. Not the best"

elif self.condition < 3:
return "Decent!"

elif self.condition < 4:
return "Pretty good"

elif self.condition <= 5:
return "Good"

else:
return "Try again!"

Choose a reason for hiding this comment

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

Nice job defining this here once and having the subclasses inherit from it.

Comment on lines +5 to +6
self.inventory = copy.copy(inventory)

Choose a reason for hiding this comment

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

I like how you make a copy of the inventory here! This is a good defensive programming practice that prevents others from messing around with our inventory inadvertently after passing it to us.

Comment on lines +12 to +17
if item not in self.inventory:
return False
else:
self.inventory.remove(item)
return item

Choose a reason for hiding this comment

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

Because the if block returns, we don't need the else. we could just write:

    def remove(self, item):
        if item not in self.inventory: 
            return False
        self.inventory.remove(item)
        return item

Comment on lines +20 to +22
items = [item for item in self.inventory if item.category == category]
return items

Choose a reason for hiding this comment

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

Nice use of a comprehension! One small thing: because we immediately return the variable, we could just say

return [item for item in self.inventory if item.category == category]

Comment on lines +37 to +46
if len(self.inventory) == 0 or len(other_vendor.inventory) == 0:
return False

vendor_first_item = self.inventory[0]
friend_first_item = other_vendor.inventory[0]

self.swap_items(other_vendor, vendor_first_item, friend_first_item)

return True

Choose a reason for hiding this comment

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

Great re-use of your earlier functions!

Comment on lines +49 to +55
items = self.get_by_category(category)
best_item = None
for item in items:
if best_item is None or item.condition > best_item.condition:
best_item = item
return best_item

Choose a reason for hiding this comment

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

Great logic here!

Comment on lines +80 to +90
assert result
assert len(tai.inventory) == 3
assert len(jesse.inventory) == 3

assert item_f in tai.inventory
assert item_a in tai.inventory
assert item_b in tai.inventory

assert item_d in jesse.inventory
assert item_e in jesse.inventory
assert item_c in jesse.inventory

Choose a reason for hiding this comment

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

Nice asserts!

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