Sea Turtles - Olive - swap meet#107
Conversation
kelsey-steven-ada
left a comment
kelsey-steven-ada
left a comment
There was a problem hiding this comment.
🎉 You completed Unit 1! 🎉 Great work on this project, I've left some suggestions for refactoring. Take a look, and let me know if anything needs clarification!
| def condition_description(self): | ||
| descriptions = ["no comment", "eh", "okay", "good", "great"," perfect, mwah"] | ||
| return descriptions[int(self.condition)] |
There was a problem hiding this comment.
Love this as a succinct solution that works for decimals as well as integers! I'd consider formatting the list a little differently to make it a bit easier to read, even through it take more lines:
| def condition_description(self): | |
| descriptions = ["no comment", "eh", "okay", "good", "great"," perfect, mwah"] | |
| return descriptions[int(self.condition)] | |
| def condition_description(self): | |
| descriptions = [ | |
| "no comment", | |
| "eh", | |
| "okay", | |
| "good", | |
| "great", | |
| "perfect, mwah" | |
| ] | |
| return descriptions[int(self.condition)] |
| pass | ||
| from swap_meet.item import Item | ||
|
|
||
| class Electronics(Item): |
| inventory = inventory if inventory is not None else [] | ||
| self.inventory = inventory |
There was a problem hiding this comment.
Nice handling of the None default value using a Ternary operator! I generally stay away from overwriting the data we get from our parameters in case we ever need that info back. In this case there's no harm, but if we wanted to avoid overwriting the inventory parameter we could use the ternary expression to directly assign self.inventory's value:
| inventory = inventory if inventory is not None else [] | |
| self.inventory = inventory | |
| self.inventory = inventory if inventory is not None else [] |
| items_by_category = [] | ||
| for item in self.inventory: | ||
| if item.category == category: | ||
| items_by_category.append(item) |
There was a problem hiding this comment.
Notice this pattern of:
result_list = []
for element in source_list:
if some_condition(element):
result_list.append(element)can be rewritten using a list comprehension as:
result_list = [element for element in source_list if some_condition(element)]Which here would look like:
items_by_category = [item for item in self.inventory if item.category == category]At first, this may seem more difficult to read, but comprehensions are a very common python style, so try getting used to working with them!
| return items_by_category | ||
|
|
||
| def swap_items(self, other, my_item, their_item): | ||
| if my_item not in self.inventory or their_item not in other.inventory: |
There was a problem hiding this comment.
I like this check for valid data, very succinct and easy to read!
| return best_item | ||
|
|
||
|
|
||
| def swap_best_by_category(self, other, my_priority, their_priority): |
There was a problem hiding this comment.
Great approach to this function!
| assert len(vendor.inventory) == 3 | ||
| assert result == False No newline at end of file |
There was a problem hiding this comment.
Great consideration to check the inventory's length hasn't changed. We could also confirm there were no side affects that altered the individual contents of the inventory with :
assert all(item in vendor.inventory for item in ["a", "b", "c"])or by individually checking values:
assert "a" in vendor.inventory
assert "b" in vendor.inventory
assert "c" in vendor.inventory| assert len(items) == 0 | ||
| assert items == [] |
There was a problem hiding this comment.
I would say both of these asserts check the same thing (whether items is empty). Since we want to know that it is a list and it is empty, we could just keep the second check assert items == [].
Aside from confirming the list is empty, what could we assert to make sure the vendor's inventory was not changed?
| assert item_f in tai.inventory | ||
| assert item_c in jesse.inventory | ||
| assert item_c not in tai.inventory | ||
| assert item_f not in jesse.inventory |
There was a problem hiding this comment.
It's great that the asserts check on the items that were swapped, but we should add asserts about the other items in the lists too, to make sure there were no unexpected side affects.
| 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.
Great checks for all relevant data!
2 participants
No description provided.