Snow Leopards- Ja Hopkins #105
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,10 @@ | ||
| from swap_meet.item import Item | ||
|
|
||
| class Clothing(Item): | ||
|
|
||
| def __init__(self, condition = 0): | ||
|
There was a problem hiding this comment. Nice job removing There was a problem hiding this comment. Typically, for default values we leave the spaces off around the def __init__(self, condition=0): |
||
| super().__init__(category = "Clothing", condition = condition) | ||
|
There was a problem hiding this comment. 👍 Great use of There was a problem hiding this comment. When calling methods with keyword arguments, we tend to avoid using spaces around the super().__init__(category="Clothing", condition=condition) |
||
|
|
||
|
|
||
| def __str__(self): | ||
|
|
||
| return ("The finest clothing you could wear.") | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,9 @@ | ||
| from swap_meet.item import Item | ||
|
|
||
| class Decor(Item): | ||
|
|
||
|
|
||
| def __init__(self, condition = 0): | ||
| super().__init__(category = "Decor", condition = condition) | ||
|
|
||
| def __str__(self): | ||
| return ("Something to decorate your space.") | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,9 @@ | ||
| from swap_meet.item import Item | ||
|
|
||
| class Electronics(Item): | ||
|
|
||
|
|
||
| def __init__(self, condition = 0): | ||
| super().__init__(category = "Electronics", condition = condition) | ||
|
|
||
| def __str__(self): | ||
| return ("A gadget full of buttons and secrets.") | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,22 @@ | ||
| class Item: | ||
|
|
||
| def __init__(self, category= "", condition = 0): | ||
|
|
||
| self.category = category | ||
| self.condition = condition | ||
|
|
||
| def __str__(self): | ||
|
There was a problem hiding this comment. 👍 This being here mostly just proves that we can replace an implementation in child classes. |
||
| return ("Hello World!") | ||
|
|
||
|
|
||
| # conditional statements | ||
|
|
||
| def condition_description(self): | ||
| if self.condition == 5: | ||
| description = "Perfect" | ||
| elif 0 <= self.condition <= 2: | ||
| description = "Used with minor flaws" | ||
| elif 2.1 <= self.condition <= 3: | ||
| description = "Used with little to no flaws" | ||
| elif 3.1 <= self.condition <= 4.9: | ||
| description = "Like New" | ||
| return description | ||
|
Comment on lines
+13
to
+22
There was a problem hiding this comment. Notice that the only lines not covered by tests are related to this function. Due to the explicit logic checks, we would need to add additional test cases to ensure that each of the possible descriptions is returned for each possible condition, since each condition could potentially have a bug in it. Explicitly writing the logic does give us the ultimate control over exactly under which situations a particular description should be used, but especially when we have a long list of chained checks that are very similar, we could try coming up with a way to represent it with data and use iteration. A basic example of this that would closely mirror your existing behavior (the ranges aren't exactly matched, and the existing implementation has a few gaps, such as at 3.05) could look like: # outside class definition
DESCRIPTIONS = (
"Used with minor flaws",
"Used with minor flaws",
"Used with minor flaws",
"Used with little to no flaws",
"Like New",
"Perfect",
)
from math import ceil
# in Item class definition
def condition_description(self):
return DESCRIPTIONS[ceil(self.condition)]We would still want to think about the edge case behavior (what if the condition is less than 0 or greater than 5), but we would have more general confidence that if this logic works for a particular condition, it's likely to work for all of them. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,68 @@ | ||
| from swap_meet.item import Item | ||
|
There was a problem hiding this comment. 👀 We don't need to import Item into this file since we never use it by name. Check out the Why Vendor Doesn't Need to Import Item for more about why. |
||
| class Vendor: | ||
|
|
||
| def __init__(self, inventory=None): | ||
| # self.inventory = inventory | ||
| if inventory is None: | ||
|
There was a problem hiding this comment. 👍 Nice handling of the default inventory case. |
||
| inventory = [] | ||
| self.inventory = inventory | ||
|
|
||
| def add(self, item): | ||
| self.item = item | ||
|
There was a problem hiding this comment. 👀 We don't need to store a reference to the And in general, we should avoid assigning to |
||
| self.inventory.append(item) | ||
| return item | ||
|
There was a problem hiding this comment. 👀 Leave a blank line between methods. |
||
| def remove(self, item): | ||
| self.item = item | ||
|
There was a problem hiding this comment. 👀 No need to store the |
||
| if item in self.inventory: | ||
| self.inventory.remove(item) | ||
| return item | ||
| return False | ||
|
Comment on lines
+16
to
+19
There was a problem hiding this comment. Prefer writing the error handling case as a guard clause first, which lets us then emphasize the main part of the code by not indenting it. if item not in self.inventory:
return False
self.inventory.remove(item)
return itemWe could also do this with a more EAFP approach by using |
||
|
|
||
| # create get_by_cateogry instance method that takes in category argument | ||
|
|
||
| def get_by_category(self, category): | ||
| # # reutrn list of items by category | ||
| self.category = category | ||
|
There was a problem hiding this comment. 👀 Like the |
||
| items_in_category = [] | ||
| # instance from item class | ||
| # item_category = Item(category=self.category) | ||
| for item in self.inventory: | ||
| # item_category = Item(category=self.category) | ||
| # if item.category == item_category.category: | ||
| if item.category == category: | ||
| items_in_category.append(item) | ||
|
Comment on lines
+29
to
+33
There was a problem hiding this comment. This would be a great opportunity for a comprehension expression. We could write this as items_in_category = [item for item in self.inventory if item.category == category] |
||
| return items_in_category | ||
|
|
||
| def swap_items(self, vendor, my_item, their_item): | ||
| self.vendor = vendor | ||
| self.my_item = my_item | ||
| self.their_item = their_item | ||
|
Comment on lines
+37
to
+39
There was a problem hiding this comment. 👀 Like the |
||
| my_items = Vendor(self.inventory) | ||
| if my_item not in my_items.inventory or their_item not in vendor.inventory: | ||
| return False | ||
| my_items.remove(my_item) | ||
| my_items.add(their_item) | ||
| vendor.add(my_item) | ||
| vendor.remove(their_item) | ||
|
Comment on lines
+40
to
+46
There was a problem hiding this comment. Rather than building a new if my_item not in self.inventory or their_item not in vendor.inventory:
return False
self.remove(my_item)
self.add(their_item)
vendor.add(my_item)
vendor.remove(their_item) |
||
| return True | ||
|
|
||
| def swap_first_item(self, vendor): | ||
| self.vendor = vendor | ||
| if not self.inventory or not vendor.inventory: | ||
| return False | ||
|
Comment on lines
+51
to
+52
There was a problem hiding this comment. 👍 Here, we do need to ensure that the lists aren't empty before trying to access index 0. And consider adding a blank line at the end of the condition block to help visually separate the the guard clause from the main logic. |
||
| return self.swap_items(vendor, self.inventory[0], vendor.inventory[0]) | ||
|
There was a problem hiding this comment. 👍 Nice function reuse. Though notice that due to the remove calls in By taking a different approach with |
||
|
|
||
|
|
||
|
|
||
| # items_in_category = [] | ||
| # for i in self.inventory: | ||
| # if i == Item(category=category): | ||
| # items_in_category.append(i) | ||
|
|
||
| # return items_in_category | ||
|
|
||
|
|
||
| # Item.category | ||
|
|
||
|
|
||
|
|
||
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.
👍 Since we are inheriting from
Item, which requires us to get theItemname from theitemmodule, we need to import this.