Conversation
yangashley
left a comment
yangashley
left a comment
There was a problem hiding this comment.
Nice work, Arika and Alaere!
You did a nice job implementing a super class, Item, and having your sub classes inherit from it and also calling the super class's dunder init method.
Your code is Pythonic, readable, and variables are descriptively named!
| @@ -1,2 +1,7 @@ | |||
| class Clothing: | |||
| pass No newline at end of file | |||
| from swap_meet.item import Item | |||
There was a problem hiding this comment.
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 name swap_meet
| pass No newline at end of file | ||
| 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 category from the available initializer parameters. Doing so indicates that we don't want the caller to be able to set a different category.
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):| def __init__(self, category = '', condition = 0, age = 0): | ||
| self.category = category | ||
| self.condition = condition | ||
| self.age = age |
| 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 |
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] | pass No newline at end of file | ||
|
|
||
| def __init__(self,inventory=None): | ||
| self.inventory = inventory if inventory is not None else [] |
| 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 |
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 inventory attribute on each Vendor object and using the Python List methods remove and append). You can reuse those methods on lines 23-26:
self.remove(my_item)
friend.remove(their_item)
self.add(their_item)
friend.add(my_item)| if self.inventory and friend.inventory: | ||
| return self.swap_items(friend, self.inventory[0], friend.inventory[0]) |
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) ?
| categories = self.get_by_category(category) | ||
|
|
||
| if not categories: | ||
| return None | ||
| return max(categories, key = lambda x : x.condition) |
There was a problem hiding this comment.
Nicely done. Consider renaming x to item for clarity
|
|
||
| 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.
| assert result | ||
| assert len(jesse.inventory) == 3 | ||
| assert len(tai.inventory) == 3 | ||
| assert [item_a, item_b, item_f] == tai.inventory |
There was a problem hiding this comment.
Nice! I think this may be just my opinion, but since we want to check that Tai's inventory contains items a, b, f I think this assertion would read more easily as:
assert tai.inventory == [item_a, item_b, item_f]
Just my opinion, very minor nit pick. Overall, nice robust test assertions!
3 participants
No description provided.