C22/ Luibov&Natalie/ swap-meet#6
Conversation
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Looks good! Please review my comments, and let me know if you have any questions via Slack, our next 1:1, or office hours. Nice job!
| result = vendor.remove(item) | ||
|
|
||
| raise Exception("Complete this test according to comments below.") | ||
| assert result == False |
There was a problem hiding this comment.
👍 This is the expected result, but we might also include checks to make sure that the vendor's inventory wasn't modified.
| assert result == False | ||
| assert len(fatimah.inventory) == 3 | ||
| assert item_a in fatimah.inventory | ||
| assert item_b in fatimah.inventory | ||
| assert item_c in fatimah.inventory | ||
| assert len(jolie.inventory) == 0 |
There was a problem hiding this comment.
👍 Great job checking both the result and that no changes happened to the inventories.
| items = vendor.get_by_category("Electronics") | ||
|
|
||
| raise Exception("Complete this test according to comments below.") | ||
| assert items == [] |
There was a problem hiding this comment.
👍 In tests, I like the asserts to be as specific as we can make them. We are told this should return an empty list in this situation, and your check literally checks for an empty list. This ensures the type is correct (a list) and that it has no contents. In contrast, if we had checked that the len of the result was 0, that would just tell us that there was a collection with notihing in it, not that it was a list. And doing a falsy test (like we usually do in application code to check for empty collections) wouldn't even guarantee that we had gotten a collection back at all, only that the result was falsy.
In earlier cases, checked that the inventories weren't modified for a failing case, but since we don't expect get_by_category to ever modify any data, we don't need to consider that here.
| assert result == True | ||
| assert len(tai.inventory) == 3 | ||
| assert len(jesse.inventory) == 3 |
There was a problem hiding this comment.
Here we should also check that items we expected to swap actually did so, and that the items that should have remained where they were also did so.
| assert result is True | ||
| assert len(tai.inventory) == 3 | ||
| assert len(jesse.inventory) == 3 | ||
|
|
||
| assert item_b in tai.inventory | ||
| assert item_a in tai.inventory | ||
| assert item_f in tai.inventory | ||
| assert item_c not in tai.inventory | ||
|
|
||
| assert item_d in jesse.inventory | ||
| assert item_e in jesse.inventory | ||
| assert item_c in jesse.inventory | ||
| assert item_f not in jesse.inventory |
There was a problem hiding this comment.
👍 Great job checking the contents as well.
Notice that we don't need to check the not in cases, since we know that each list is 3 items long, and we checked that the 3 expected to be there are in fact there, this means the 4th item (that should have swapped) cannot be there. If it were there, that would mean there were 4 items in the inventories.
| return items_in_category | ||
|
|
||
| def get_best_by_category(self, category): | ||
| items_in_category = self.get_by_category(category) |
There was a problem hiding this comment.
👍 Nice use of the function we just made, and then locating the best value in the result (if any).
| def find_best_item_by_category(self, category): | ||
| return self.get_best_by_category(category) | ||
|
|
||
| def swap_best_by_category(self, other_vendor, my_priority, their_priority): |
There was a problem hiding this comment.
👍 Great job reusing our Vendor functions.
| self.swap_items(other_vendor, my_best_item, their_best_item) | ||
| return True |
There was a problem hiding this comment.
We could return the result of this function call, which will be False if the swap fails, and True if it succeeds.
| if my_best_item is None or their_best_item is None: | ||
| return False |
There was a problem hiding this comment.
Is this check necessary? If either of the find_best_item_by_category calls fails, we'll get bakc None. None shouldn't be in either of the inventories, meaning an attempt to swap would also fail, returning False.
It still might be more clear to have this check, but if we have confidence in the behavior of our other functions, we may be able to remove unnecessary logic.
| return True | ||
|
|
||
|
|
||
| def reorder_inventory(self, new_order): |
There was a problem hiding this comment.
How would you expect a function like this to be used? It looks like it allows a new ordering to be specified and a new inventory will be constructed. Did you have a particular use case in mind?
It would be great if there were tests added for this function to show what some of the behaviors could be. And for some additional validation, what would happen if not every index were included (e.g., some indices were duplicated)?
3 participants
No description provided.