-
Notifications
You must be signed in to change notification settings - Fork 113
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 - K Frank #111
base: master
Are you sure you want to change the base?
Whales - K Frank #111
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job K! Very nice use of inheritance and re-use of methods. You've written some really DRY code here! This submission is green~
def __init__(self, category = "Clothing", condition = 0): | ||
super().__init__(category, condition) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great use of super
here! Consider removing the category
from the constructor arguments on line 3. We know we always want it to be "Clothing" so we don't want to allow changing it when instantiating an instance.
def __init__(self, category = "Electronics", condition = 0): | ||
super().__init__(category, condition) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny style point here and elsewhere: when defining default arguments don't include spaces on either side of the equals sign. We would instead write it like:
def __init__(self, category="Electronics", condition=0):
if self.condition == 0: | ||
return "Poor" | ||
if self.condition == 1: | ||
return "Fair" | ||
if self.condition == 2: | ||
return "Good" | ||
if self.condition == 3: | ||
return "Very good" | ||
if self.condition == 4: | ||
return "Like new" | ||
if self.condition ==5: | ||
return "New" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job having this be defined here once and inherited by the child classes!
'''Adds an item to a vendor's inventory''' | ||
self.inventory.append(item) | ||
return item |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great docstrings here and elsewhere!
if not inventory: | ||
inventory = [] | ||
self.inventory = inventory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job avoiding a mutable default object.
"Returns a list of items of a category in a vendor's inventory" | ||
item_list = [item for item in self.inventory if item.category == category] | ||
return item_list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great comprehension!
'''Swaps the first item in two vendor inventories (self and vendor)''' | ||
if self.inventory and vendor.inventory: | ||
return self.swap_items(vendor, self.inventory[0], vendor.inventory[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love how you re-use your earlier method! One small bug here: according to the README this method should return False
if either of the inventories is empty, but it actually returns None
.
'''Returns the best item in vendor inventory by category. If multiple items, returns the item with the highest condition''' | ||
items_list = self.get_by_category(category) | ||
if not items_list: | ||
return None | ||
best_item = items_list[0] | ||
for item in items_list: | ||
if item.condition > best_item.condition: | ||
best_item = item | ||
return best_item |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great logic and re-use of method here!
assert result == "c" | ||
assert vendor.inventory == ["a", "b"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice asserts!
# - That the results is truthy | ||
assert not result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you may have accidentally copied a comment that says the opposite of what you want.
No description provided.