SeaTurtles_AshleyBenson #110
Conversation
There was a problem hiding this comment.
Nice job!
All required waves are complete and all tests are passing (including the tests which need completion)! I did need to make a small correction to a test where an unnecessary change was made that was leading to a test failure. Also, be sure to keep an eye out for the integration tests, and unskip those as well!
Your code is well organized, and you are generally using good, descriptive variable names. Your general approaches are good, you're off to a good start with OOP, and you're making good reuse of functions.
The main things I would recommend focusing are are in some of the smaller details. For instance, try to write your code in more pythonic ways. Avoiding unnecessary indenting (with guard clauses), and try to make use of list comprehensions. They may seem more difficult to read now, but they will become more familiar over time, and other developers often expect code to be written that way.
Overall, well done.
| def test_removing_from_inventory_returns_item(): | ||
| item = "item to remove" | ||
| vendor = Vendor( | ||
| inventory=["a", "b", "c", item] | ||
| inventory=["a", "b", "c", "item"] |
There was a problem hiding this comment.
This test was failing.
item here is the variable initialized on line 32 (the string "item to remove"). Adding quotes around this causes the test to fail, since rather than adding "item to remove" to the list, and removing it later, it adds "item" to the list, which then doesn't match the act or assert statements as written.
Restoring this line back to item, the test passes.
| assert len(vendor.inventory) == 3 | ||
| assert item not in vendor.inventory | ||
| assert result == False |
There was a problem hiding this comment.
👍
I like the length test to ensure that all the items are still there. We don't really need the check for item not being in the list, since it was never in the list. And I like the explicit False check here, rather than something a little more falsy.
We could still consider checking that each of the three things that were in the list at the start are still there (that there were no shenanigans from the remove), but given that you're checking the overall length already, that's largely a matter of personal preference.
| @@ -33,8 +33,8 @@ def test_get_no_matching_items_by_category(): | |||
| ) | |||
|
|
|||
| items = vendor.get_by_category("electronics") | |||
|
|
|||
| raise Exception("Complete this test according to comments below.") | |||
| assert len(items) == 0 | |||
| @@ -1,10 +1,11 @@ | |||
| from unittest import result | |||
There was a problem hiding this comment.
Watch out for stray imports like this. VSCode can end up adding these if we're not careful with the suggestions it makes.
| @@ -75,8 +76,12 @@ def test_swap_best_by_category(): | |||
| my_priority="Clothing", | |||
| their_priority="Decor" | |||
| ) | |||
|
|
|||
| raise Exception("Complete this test according to comments below.") | |||
| #assert result == True | |||
There was a problem hiding this comment.
I would include this check that the result is True. Returning True on successful swap is one of the behaviors defined for this function.
| if not self.inventory: | ||
| return None | ||
| else: | ||
| items = self.get_by_category(category) | ||
| if items: return max( items, key=lambda item: item.condition ) |
There was a problem hiding this comment.
Nice use of max with a key function to help find the item with the "best" condition. We've seen enough python now, that you could try writing your own version of max, including an optional key function parameter. Give it a shot!
Also, since get_by_category returns an empty list if it can't find any items of the desired category, we can do away with the initial empty list check. However, we do need to to either check that thee items result isn't empty before using it with max, or else max will raise an error (or we could handle the error!).
From a style perspective, avoid squeezing if statements and their condition onto a single line. Also, make sure that both branches of the if end up returning something if one of them does. The existing code was correctly returning None as expected when nothing could be found, but this is because of the default Python behavior of returning None when there is no return statement. When one branch is returning something, it looks like an oversight to not return from the other branch, so make sure to explicitly return None if needed (here, rearranged under a guard clause).
def get_best_by_category(self, category):
items = self.get_by_category(category)
if not items:
return None
return max( items, key=lambda item: item.condition )|
|
||
|
|
||
| friend_swap = other.get_best_by_category(my_priority) | ||
| my_swap = self.get_best_by_category(their_priority) | ||
|
|
||
| if not friend_swap: | ||
| return False | ||
| if not my_swap: | ||
| return False | ||
|
|
||
| self.swap_items(other, my_swap, friend_swap) | ||
| return True |
There was a problem hiding this comment.
👍
Really nice function reuse. Notice that swap_items already shares the same return behavior as we would like here (and would properly handle None being passed in as either param; convince yourself why this is so). So we could even condense this down to
def swap_best_by_category(self, other, my_priority, their_priority):
friend_swap = other.get_best_by_category(my_priority)
my_swap = self.get_best_by_category(their_priority)
return self.swap_items(other, my_swap, friend_swap)| # return False | ||
| # elif my_priority not in other.inventory: | ||
| # return False |
There was a problem hiding this comment.
Be sure to clean up old commented out code
| super().__init__(condition = condition, category = "Clothing") |
There was a problem hiding this comment.
Nice job removing the category from the constructor, since we don't want to encourage the caller to try to change it. Instead, we pass the literal value that we want to set up to the parent's constructor
With respect to formatting, keep in mind that PEP8 recommends no spaces around = when used for default values or keyword arguments. So we might prefer to write
def __init__(self, condition=0.0):
super().__init__(condition=condition, category="Clothing")| super().__init__(condition = condition, category = "Clothing") | ||
|
|
||
| def __str__(self): |
No description provided.