Conversation
yangashley
left a comment
yangashley
left a comment
There was a problem hiding this comment.
Nice job, Sana!
All required waves are complete and all required tests are passing! Your code is well organized and you used descriptive variable names. Your general approaches are good, and you're off to a good start with OOP.
I called out some places where you can reverse your logic so you implement a guard clause instead of if/else. This will make your python code more idiomatic. Avoiding unnecessary indenting (by using guard clauses), and checking conditions making use of pythons ideas about truthy and falsy can communicate your intent to other python developers very quickly.
I also pointed out where you can use list comprehension and list destructuring if you're up for it.
Well done 🟢 !
| pass No newline at end of file | ||
| from swap_meet.item import Item | ||
|
|
||
| class Clothing(Item): |
| @@ -1,2 +1,15 @@ | |||
| class Item: | |||
| if not inventory: | ||
| self.inventory = [] | ||
| else: | ||
| self.inventory = inventory |
There was a problem hiding this comment.
Since you have a guard clause on line 3, you don't need the dangling else on line 5.
You could write it like:
if not inventory:
self.inventory = []
self.inventory = inventoryYou might also see this written as a ternary:
my_var = "some value" if some_condition else "other value"
In this situation, that would look like
self.inventory = [] if inventory is None else inventory| if item in self.inventory: | ||
| self.inventory.remove(item) | ||
| return item | ||
| return False |
There was a problem hiding this comment.
A more explicit way to return False would be to use a guard clause instead doing it in the main body of the method.
if item not in self.inventory:
return False
self.inventory.remove(item)
return itemAnother approach we could take is to try to remove the item directly, and handle the ValueError that occurs if it's not there, and return False to handle it (try/except)
| item_list = [] | ||
| for item in self.inventory: | ||
| if category == item.category: | ||
| item_list.append(item) | ||
|
|
||
| return item_list |
There was a problem hiding this comment.
Looks good.
This method is a great candidate for using list comprehension if you want to refactor it since our conditional statement we check is pretty simple.
General syntax for list comp:
result_list = [element for element in source_list if some_condition(element)]Which here would look like:
item_list = [item for item in self.inventory if item.category == category]You can also forgo saving the list to the variable item_list and do something like:
def get_by_category(self, category):
return [item for item in self.inventory if item.category == category]| if my_item not in self.inventory: | ||
| return False | ||
| if their_item not in vendor.inventory: | ||
| return False |
There was a problem hiding this comment.
Nice guard clause. You can combine lines 27 and 29 like:
if my_item not in self.inventory and their_item not in vendor.inventory:
return false
| self.add(their_item) | ||
| return True | ||
|
|
||
| def swap_first_item(self, vendor: 'Vendor'): |
| for item in self.inventory: | ||
| if item.category == category: | ||
| category_items.append(item) |
There was a problem hiding this comment.
Consider reusing your get_by_category method that you already defined so you don't have to loop and add to a list again.
| my_item = self.get_best_by_category(their_priority) | ||
| their_item = other.get_best_by_category(my_priority) | ||
|
|
||
| return self.swap_items(other, my_item, their_item) |
There was a problem hiding this comment.
Nice work reusing your function get_best_by_category.
Also, way to leverage the validation checks in swap_items method on line 27 and 29 in Vendor.py.
I also like that you return the Boolean value from swap_items so you didn't have to explicitly return True or return False here in swap_best_by_category.
| assert len(tai.inventory) ==3 | ||
| assert len(jesse.inventory) == 3 | ||
| assert item_a in tai.inventory | ||
| assert item_b in tai.inventory | ||
| assert item_c in tai.inventory | ||
| assert item_d in jesse.inventory | ||
| assert item_e in jesse.inventory | ||
| assert item_f in jesse.inventory |
There was a problem hiding this comment.
You can combine lines 252 and 253 if you want:
assert len(tai.inventory) == 3 and len(jesse.inventory) == 3
You can use list destructing like this too - a pythonic way of doing it:
assert [item_a, item_b, item_c] == tai.inventory
assert [item_d, item_e, item_f] == jesse.inventoryhttps://medium.com/@umaramanat66/destructuring-list-in-python-like-javascript-f7d4c0968538
2 participants
No description provided.