Ada C22 Phoenix Class - Madina Dzhetegenova#20
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!
| assert item not in vendor.inventory | ||
| assert result == False |
There was a problem hiding this comment.
👍 While it would be surprising if the remove call had added item into the inventory (remove doesn't have any logic that should result in this being possible), I like that you're thinking about checking the status of the inventory. Since the remove failed, there shold be no change. So we might wantt o ensure that the values that started in the inventory were all still there after the remove call.
| # ********************************************************************* | ||
| # ****** Complete Assert Portion of this test ********** | ||
| # ********************************************************************* | ||
| assert result == False |
There was a problem hiding this comment.
👍 Here again, this is the expected result, but we might also include checks to make that the vendor's inventory wasn't modified after the failed swap.
| items = vendor.get_by_category("Electronics") | ||
|
|
||
| raise Exception("Complete this test according to comments below.") | ||
| assert len(items) == 0 |
There was a problem hiding this comment.
👍 We shouldn't get any matches in this case.
Notice that this checks that items is an empty collection, but not necessarily a list. The function is specified to return an empty list when no items are found, so we might prefer to write this as
assert items == []Or we could add a type()/isinstance() assertion that items is, in fact, a list.
| ) | ||
|
|
||
| raise Exception("Complete this test according to comments below.") | ||
| assert result is True |
There was a problem hiding this comment.
👍 I agree with checking literally for True rather than doing a truthy check, since the spec literally says the result should be True. In application code that uses this function, we might relax our checks to be more truthy/falsy, but a test should enforce the behavior as strictly as possible.
Typically, if we do need to compare with True or False we usually use == (and is for None). Technically, == wouldn't guarantee that result was literally True (in Python 0 == False and 1 == True), so I could be persuaded to prefer is since we're expecting to literally return either True or False.
| assert item_c not in tai.inventory | ||
| assert item_f in tai.inventory | ||
| assert item_f not in jesse.inventory | ||
| assert item_c in jesse.inventory |
There was a problem hiding this comment.
👀 We should also make sure that the non-swapped items are still in their original lists. Yes, we checked that the overall length hasn't changed, but we should make sure that the membership is as we expect.
| items_in_category = [] | ||
| for item in self.inventory: | ||
| if item.get_category() == category: | ||
| items_in_category.append(item) |
There was a problem hiding this comment.
👍 We need to loop to find all the items with the specified category.
This would be a great opportunity to try using list comprehension syntax.
| return items_in_category | ||
|
|
||
| def get_best_by_category(self, category=None): | ||
| 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).
| for item in items_in_category: | ||
| if item.condition > item_best_condition.condition: | ||
| item_best_condition = item |
There was a problem hiding this comment.
👍 Nice logic to loop through and find the item with the best condition.
| my_best_item = self.get_best_by_category(their_priority) | ||
| their_best_item = other_vendor.get_best_by_category(my_priority) |
There was a problem hiding this comment.
👍 Great job reusing our Vendor functions.
| self.remove(my_best_item) | ||
| other_vendor.add(my_best_item) | ||
|
|
||
| other_vendor.remove(their_best_item) | ||
| self.add(their_best_item) |
There was a problem hiding this comment.
👀 Once we've identified the two items to swap, could we reuse swap logic that we previously wrote?
3 participants
No description provided.