Lions - Nancy L #101
Lions - Nancy L #101
Conversation
There was a problem hiding this comment.
There's some really great logic here! I also love how frequent your commits are. There are a few places where inheritance isn't fully taken advantage of and there are there are a few tests that aren't passing, so this project is a yellow. That being said, really nicely done with what you've got here!
swap_meet/clothing.py
Outdated
| from hashlib import new |
There was a problem hiding this comment.
Oops! Looks like VS Code accidentally auto-imported something you didn't want here. Make sure to double-check for these and deleted unneeded imports.
swap_meet/clothing.py
Outdated
| def __init__(self, category="Clothing", condition=0): | ||
| self.category = category | ||
| self.condition = condition |
There was a problem hiding this comment.
While this works, we would prefer to use super here to take advantage of the constructor for Item.
We also would prefer to not have category be an optional argument - we know that we always want the category to be "Clothing".
swap_meet/item.py
Outdated
| from distutils.archive_util import make_archive | ||
| from operator import itemgetter | ||
| import py_compile |
There was a problem hiding this comment.
Looks like a few more accidental imports from VS Code here.
swap_meet/item.py
Outdated
| class Item: | ||
| pass No newline at end of file | ||
| def __init__(self, category="", condition=0): | ||
| self.category = category | ||
| self.condition = condition | ||
|
|
||
| def __str__ (self): | ||
| return "Hello World!" |
| if self.condition == 0: | ||
| return "poor" | ||
| elif self.condition == 1: | ||
| return "fair" | ||
| elif self.condition == 2: | ||
| return "good" | ||
| elif self.condition == 3: | ||
| return "excellent" | ||
| elif self.condition == 4: | ||
| return "perfect" | ||
| elif self.condition == 5: | ||
| return "new" |
There was a problem hiding this comment.
This works well if the condition is an integer, but what would happen if the condition was a float like 3.5 as it is in some of the tests? Can you think of how you would solve this issue?
| if len(self.inventory) == 0 or len(friend.inventory) == 0: | ||
| return False | ||
| else: | ||
| self.swap_items(friend, my_item=self.inventory[0], their_item=friend.inventory[0]) | ||
| return True |
swap_meet/vendor.py
Outdated
| # loop through the self.inventory | ||
| best_item = None | ||
| for item in self.get_by_category(category): | ||
| if best_item == None: | ||
| best_item = item | ||
| elif item.condition > best_item.condition: | ||
| best_item = item | ||
| return best_item |
There was a problem hiding this comment.
Really nice logic and awesome re-use of get_by_category! One small change, use is instead of == to check whether something is None.
| assert not result | ||
| # - That tai and jesse's inventories are the correct length | ||
| assert len(tai.inventory) == 3 | ||
| assert len(jesse.inventory) == 3 | ||
| # - That all the correct items are in tai and jesse's inventories | ||
| assert item_c in tai.inventory | ||
| assert item_b in tai.inventory | ||
| assert item_a in tai.inventory | ||
| assert item_f in jesse.inventory | ||
| assert item_e in jesse.inventory | ||
| assert item_d in jesse.inventory |
swap_meet/vendor.py
Outdated
| from multiprocessing import Condition | ||
| from unicodedata import category |
There was a problem hiding this comment.
A few more accidental VS Code imports.
tests/unit_tests/test_wave_06.py
Outdated
| assert result is True | ||
| # - That tai and jesse's inventories are the correct length | ||
| assert len(tai.inventory) == 3 | ||
| assert len(jesse.inventory) == 3 | ||
| # - That all the correct items are in tai and jesse's inventories, including the items which were swapped from one vendor to the other | ||
| 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.
I like the idea of these assertions, but I think there's a bug here. Your assertions say that tai and jesse still have the exact same items they started with. However, we want them to have swapped the best item by category.
| def __init__(self, category="Clothing", condition=0): | ||
| self.category = category | ||
| self.condition = condition | ||
| super().__init__(category, condition) |
There was a problem hiding this comment.
Great use of __super__! Still consider my previous comment about why we might not want category to be an argument on the Clothing class.
| @@ -21,7 +18,7 @@ def remove(self, item): | |||
| def get_by_category(self, category): | |||
| merch =[] | |||
| for item in self.inventory: | |||
| if item.category is category: | |||
| if item.category == category: | |||
| best_item = None | ||
| for item in self.get_by_category(category): | ||
| if best_item == None: | ||
| if best_item is None: |
| my_best_item = self.get_best_by_category(their_priority) | ||
| their_best_item = other.get_best_by_category(my_priority) | ||
| if their_best_item is None or my_best_item is None: | ||
| return False | ||
| else: | ||
| self.swap_items(other, my_best_item, their_best_item) | ||
| return True |
There was a problem hiding this comment.
Great job! Really nice re-use of your earlier methods here. We don't need the else here, can you consider why?
There was a problem hiding this comment.
Instead of using else I could just use another if statement. Since line 62 returns False, it exits the function. I realized that just now :)
No description provided.