Conversation
anselrognlie
left a comment
anselrognlie
left a comment
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)! Your code is well organized, you are using good, descriptive variable names, and you had good code reuse.
There are a few places where it looks like you could review some OOP concepts a bit (the use of self in instance methods, class variables which we didn't particularly need in this project, and what is really happening when we call a parent's __init__).
Otherwise, the main things I would recommend focusing on are some of the smaller details. Watch for places where your code could be a little more pythonic (guard clauses to reduce nesting, list comprehensions). Also, try looking for patterns of repeated code, and even though a helper function might not be a good fit, think about whether putting the repeated data into a collection of some kind might help us write code that's more DRY (and data-driven!).
Overall, well done!
| 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.
| #raise Exception("Complete this test according to comments below.") | ||
|
|
||
|
|
||
| assert items == [] |
There was a problem hiding this comment.
👍
This always looks strange to me since I always want to write empty list checks as not items, but here in the tests we want to be specific like this. In application Python code, we should use the more permissive checks (not items), but in a test, we want to ensure that the thing returned really is a list and that it is empty.
| # - That tai and jesse's inventories are the correct length | ||
| # - 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 result == True |
| # - That tai and jesse's inventories are the correct length | ||
| # - That all the correct items are in tai and jesse's inventories, and that the items that were swapped are not there | ||
|
|
||
| assert result is True |
| # - That result is falsy | ||
| # - That tai and jesse's inventories are the correct length | ||
| # - That all the correct items are in tai and jesse's inventories | ||
| assert result is False |
| if my_item not in self.inventory or their_item not in other_vendor.inventory: | ||
| return False |
There was a problem hiding this comment.
👍
Great use of a guard clause to make sure that both vendors are in a valid configuration for performing the swap.
We could still try to use error handling here, but with the two separate remove calls, if the second one failed, we would need to to additional work to restore the condition of the inventories, or take a different approach.
| else: | ||
| self.swap_items(other_vendor, self.inventory[0], other_vendor.inventory[0]) | ||
| return True |
There was a problem hiding this comment.
Since the initial check above returns when the needed conditions (non-empty inventories) aren't met, we can treat it as a guard clause, removing the explicit else, and unindenting the following "main" logic.
self.swap_items(other_vendor, self.inventory[0], other_vendor.inventory[0])
return True|
|
||
| best_condition_so_far = 0 | ||
|
|
||
| list_of_items_by_category = self.get_by_category(category) |
There was a problem hiding this comment.
👍 Nice function reuse to start off with a list of inventory items filtered to the desired category.
| for item in list_of_items_by_category: | ||
| if item.condition > best_condition_so_far: | ||
| best_condition_so_far = item.condition | ||
| for item in list_of_items_by_category: | ||
| if item.condition == best_condition_so_far: | ||
| return item |
There was a problem hiding this comment.
We could do these two loops "at once". We could have a second variable that tracks the best item found so far (initialize it to None) and each time we update the condition, also update the item. Then at the end of the loop, our second variable would already have the best item and we could return it immediately.
Note that from a big O perspective, what you've written here and what I described are both O(n) with respect to the number of items. Your approach would translate more directly into an approach using max.
| self_best_item = self.get_best_by_category(their_priority) | ||
| their_best_item = other.get_best_by_category(my_priority) | ||
|
|
||
| return self.swap_items(other, self_best_item, their_best_item) |
There was a problem hiding this comment.
👍
We can reuse several of our existing functions to handle this very cleanly. Remember that get_best_by_category can return None, so be sure to convince yourself why swap_items will work as we would like, even if we pass None in for one or both of the arguments.
2 participants
No description provided.