Conversation
…e method for Item class
… until they were created.
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Nice job on the Item inheritance. There are a few scattered things to think about, such as other approaches to the condition description method implementation. Also, I'm curious what the reason was for assigning parameter values to self attributes in many of the Vendor methods. And pay particular attention to my comments on swap_items as well.
| @pytest.mark.skip | ||
| @pytest.mark.integration_test | ||
| # @pytest.mark.skip | ||
| # @pytest.mark.integration_test |
There was a problem hiding this comment.
The integration test decorator should not have been commented out. This mark affects the order that tests are run, as well as how the code coverage is calculated.
| # ********************************************************************* | ||
| # ****** Complete Assert Portion of this test ********** | ||
| # ********************************************************************* | ||
| assert result == False No newline at end of file |
There was a problem hiding this comment.
👍 While we wouldn't typically compare directly with False in Python code, if we want to confirm that the result is False, as required in the description (rather than just something falsy) then we can write this. We also may want to ensure that the length of the vendor's inventory was unchanged by the operation.
Personally, I don't like the design the description asks for (I'd rather it either return None or raise an error), but 🤷 .
| assert items == [] | ||
| assert len(items) == 0 | ||
| assert item_a not in items | ||
| assert item_b not in items | ||
| assert item_c not in items |
There was a problem hiding this comment.
👍 The comparison against the empty list is really all we need. If the list is empty, none of those items can possibly be in it.
As before, we usually don't explicitly check for an empty collection like this, but since the unit test is confirming the specified behavior, which was to return a list (not just something falsy), we can write it this way.
| @@ -1,2 +1,10 @@ | |||
| class Clothing: | |||
| pass No newline at end of file | |||
| from swap_meet.item import Item | |||
There was a problem hiding this comment.
👍 Since we are inheriting from Item, which requires us to get the Item name from the item module, we need to import this.
|
|
||
| class Clothing(Item): | ||
|
|
||
| def __init__(self, condition = 0): |
There was a problem hiding this comment.
Nice job removing category from the available initializer parameters. We don't want the caller to be able to set a different category. Though, the caller could always change the category by assigning directly to it after. But at least we're indicating that they shouldn't do that.
There was a problem hiding this comment.
Typically, for default values we leave the spaces off around the =
def __init__(self, condition=0):| self.vendor = vendor | ||
| self.my_item = my_item | ||
| self.their_item = their_item |
There was a problem hiding this comment.
👀 Like the self.item cases above, these assignments are not needed and should be avoided.
| my_items = Vendor(self.inventory) | ||
| if my_item not in my_items.inventory or their_item not in vendor.inventory: | ||
| return False | ||
| my_items.remove(my_item) | ||
| my_items.add(their_item) | ||
| vendor.add(my_item) | ||
| vendor.remove(their_item) |
There was a problem hiding this comment.
Rather than building a new Vendor (which happens to share the same inventory list as the current Vendor), we can use the self instance directly.
if my_item not in self.inventory or their_item not in vendor.inventory:
return False
self.remove(my_item)
self.add(their_item)
vendor.add(my_item)
vendor.remove(their_item)| if item in self.inventory: | ||
| self.inventory.remove(item) | ||
| return item | ||
| return False |
There was a problem hiding this comment.
Prefer writing the error handling case as a guard clause first, which lets us then emphasize the main part of the code by not indenting it.
if item not in self.inventory:
return False
self.inventory.remove(item)
return itemWe could also do this with a more EAFP approach by using remove or index directly and handling the error that would get raised if the item were not present (ValueError).
| if not self.inventory or not vendor.inventory: | ||
| return False |
There was a problem hiding this comment.
👍 Here, we do need to ensure that the lists aren't empty before trying to access index 0.
And consider adding a blank line at the end of the condition block to help visually separate the the guard clause from the main logic.
| self.vendor = vendor | ||
| if not self.inventory or not vendor.inventory: | ||
| return False | ||
| return self.swap_items(vendor, self.inventory[0], vendor.inventory[0]) |
There was a problem hiding this comment.
👍 Nice function reuse. Though notice that due to the remove calls in swap_items, this will be O(n) where n is the lengths of the inventories, since we'll have to shift the whole remaining part of the inventories forward.
By taking a different approach with swap_items or customizing our implementation here, we could do this swap in constant time.
3 participants
No description provided.