Snow leopards- Arika A. & Alaere N. #95
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,7 @@ | ||
| from swap_meet.item import Item | ||
| class Clothing(Item): | ||
| def __init__(self, condition = 0, age = 0): | ||
|
There was a problem hiding this comment. Nice work removing The caller could always change the category by assigning directly to it after: clothing = Clothing(condition=3)
clothing.category = "Electronics"But at least we're indicating that they shouldn't do that when we organize the code this way. Also, with default params we tend to leave spaces off from each side of the equal sign: def __init__(self, condition=0, age=0): |
||
| super().__init__('Clothing', condition, age) | ||
|
|
||
| def __str__(self): | ||
| return f"The finest clothing you could wear." | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,7 @@ | ||
| from swap_meet.item import Item | ||
| class Decor(Item): | ||
| def __init__(self, condition = 0, age = 0): | ||
| super().__init__('Decor', condition, age) | ||
|
|
||
| def __str__(self): | ||
| return f"Something to decorate your space." |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,8 @@ | ||
| from swap_meet.item import Item | ||
| class Electronics(Item): | ||
| def __init__(self, condition = 0, age = 0): | ||
| super().__init__('Electronics', condition, age) | ||
|
|
||
| def __str__(self): | ||
| return "A gadget full of buttons and secrets." | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,24 @@ | ||
| class Item: | ||
| def __init__(self, category = '', condition = 0, age = 0): | ||
| self.category = category | ||
| self.condition = condition | ||
| self.age = age | ||
|
Comment on lines
+2
to
+5
|
||
|
|
||
|
|
||
| def __str__(self): | ||
| return f"Hello World!" | ||
|
|
||
| def condition_description(self): | ||
| description = { | ||
| 0: 'bad', | ||
| 1: 'poor', | ||
| 2: 'decent', | ||
| 3: 'average', | ||
| 4: 'good', | ||
| 5: 'pristine' | ||
| } | ||
|
|
||
| for rating, quality in description.items(): | ||
| if self.condition == rating: | ||
| return quality | ||
|
Comment on lines
+12
to
+23
There was a problem hiding this comment. Excellent use of a data structure here to get the description based on condition - using a list enables you to avoid a long list of chained checks (if/elif/elif...) which can introduce bugs. There are still edge case behaviors we'd need to consider and handle: what if the condition is outside the expected range? what if the condition is a non-integer value? Currently the function would return None if the condition on line 22 doesn't evaluate to True. Also, when you have a dict, you can iterate through the keys without using .items like: for rating in description:
if condition == rating:
return description[rating] |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,57 @@ | ||
| class Vendor: | ||
|
|
||
| def __init__(self,inventory=None): | ||
| self.inventory = inventory if inventory is not None else [] | ||
|
|
||
|
|
||
| def add(self,item): | ||
| self.inventory.append(item) | ||
| return item | ||
|
|
||
| def remove(self,item): | ||
| if item in self.inventory: | ||
| self.inventory.remove(item) | ||
| return item | ||
| return False | ||
|
Comment on lines
+11
to
+14
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 item |
||
|
|
||
| def get_by_category(self, category): | ||
|
|
||
| return [item for item in self.inventory if category == item.category] | ||
|
|
||
|
|
||
| def swap_items(self,friend, my_item, their_item): | ||
|
|
||
| if my_item in self.inventory and their_item in friend.inventory: | ||
| self.inventory.remove(my_item) | ||
| friend.inventory.remove(their_item) | ||
| self.inventory.append(their_item) | ||
| friend.inventory.append(my_item) | ||
| return True | ||
| return False | ||
|
Comment on lines
+22
to
+28
There was a problem hiding this comment. We can use a guard clause here too to flip the logic so that we have a check to make sure we're working with valid data. If the data isn't valid we'll return false and then the meat of your logic can be un-indented in the lines that follow: if my_item not in self.inventory or their_item not in other_vendor.inventory:
return False
# rest of your swapping logic hereAlso, recall that you already wrote methods for the Vendor class that allow you to add and remove from the inventory (without needing to access the self.remove(my_item)
friend.remove(their_item)
self.add(their_item)
friend.add(my_item) |
||
|
|
||
| def swap_first_item(self, friend): | ||
|
|
||
| if self.inventory and friend.inventory: | ||
| return self.swap_items(friend, self.inventory[0], friend.inventory[0]) | ||
|
Comment on lines
+32
to
+33
There was a problem hiding this comment. What would this function return if self.inventory and friend.inventory were empty? How could you use a guard clause here to handle that scenario? Also, it's great to reuse the swap_items method you already wrote! However, swap_items has linear time complexity because it invokes the remove() method. Can you think of how you could write a custom swapping implementation here to bring the time complexity to O(1) ? |
||
|
|
||
| def get_best_by_category(self, category): | ||
|
|
||
| categories = self.get_by_category(category) | ||
|
|
||
| if not categories: | ||
| return None | ||
| return max(categories, key = lambda x : x.condition) | ||
|
Comment on lines
+37
to
+41
There was a problem hiding this comment. Nicely done. Consider renaming |
||
|
|
||
| def swap_best_by_category(self, other, my_priority, their_priority): | ||
|
|
||
| other_vendor = other.get_best_by_category(my_priority) | ||
| my_vendor = self.get_best_by_category(their_priority) | ||
|
|
||
| return self.swap_items(other, my_vendor, other_vendor) | ||
|
|
||
| def swap_by_newest(self, other, their_newest, my_newest): | ||
|
|
||
| return self.swap_items(other, min(self.inventory, key = lambda x : x.age) , min(other.inventory, key = lambda x : x.age)) | ||
|
There was a problem hiding this comment. Nice and concise solution here! To make the method a little more readable, consider pulling out some of these values into variables and passing the variables into swap_items. It's definitely up to the coder to decide when to pull values into variables and when to write things in-line. The reason I suggested using variables is because as I was reading line 52, I had to slow down and figure out what each argument being passed into swap_items was -- that's usually a signal that variables should be introduced. |
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
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 Item and Clothing are in the same swap_meet package, we can import the class with shorter syntax
from .item import Item. If they were not in the same package, then we would need to specify the package nameswap_meet