Conversation
jbieniosek
left a comment
jbieniosek
left a comment
There was a problem hiding this comment.
Fantastic work on this project Lex! Really great use of helper methods throughout vendor. The code is very clean and easy to read. My only critical feedback is on the tests, I think being a bit more thorough and clear in what is being tested will take them to the next level. This project is green.
| def __init__(self, condition=0): | ||
| super().__init__("Clothing", condition) |
| if inventory is None: | ||
| inventory = [] | ||
| self.inventory = inventory |
| for item in self.inventory: | ||
| if item.category == category: | ||
| items.append(item) |
| self.remove(my_item) | ||
| friend_vendor.add(my_item) | ||
| friend_vendor.remove(their_item) | ||
| self.add(their_item) |
| return False | ||
| my_first_item = self.inventory[0] | ||
| their_first_item = friend_vendor.inventory[0] | ||
| is_swapped = self.swap_items(friend_vendor, my_first_item, their_first_item) |
| if item.condition > top_condition: | ||
| top_condition = item.condition | ||
| return_item = item | ||
| return return_item |
| my_best = self.get_best_by_category(their_priority) | ||
| their_best = other.get_best_by_category(my_priority) | ||
| return self.swap_items(other, my_best, their_best) |
|
|
||
| raise Exception("Complete this test according to comments below.") | ||
| #added assert | ||
| assert result == False |
There was a problem hiding this comment.
I suggest adding a check to verify that the inventory wasn't changed by the remove action:
| assert result == False | |
| assert result == False | |
| assert len(vendor.inventory) == 3 |
| assert len(tai.inventory) == len(jesse.inventory) == 3 | ||
| assert len(tai.get_by_category("Clothing")) == 1 | ||
| assert len(jesse.get_by_category("Clothing")) == 1 | ||
| assert len(tai.get_by_category("Decor")) == 1 | ||
| assert len(jesse.get_by_category("Decor")) == 2 | ||
| assert tai.get_by_category("Decor")[0].condition == 2.0 | ||
| assert tai.get_by_category("Clothing")[0].condition == 4.0 | ||
| assert jesse.get_by_category("Clothing")[0].condition == 2.0 |
There was a problem hiding this comment.
I recommend restructuring these tests to make sure the correct elements are swapped. It will make the asserts themselves a little simpler, but also check the specific action in the act step. Also, removing get_by_category reduces the possibility that a bug in that function will falsely register a bug here.
| assert len(tai.inventory) == len(jesse.inventory) == 3 | |
| assert len(tai.get_by_category("Clothing")) == 1 | |
| assert len(jesse.get_by_category("Clothing")) == 1 | |
| assert len(tai.get_by_category("Decor")) == 1 | |
| assert len(jesse.get_by_category("Decor")) == 2 | |
| assert tai.get_by_category("Decor")[0].condition == 2.0 | |
| assert tai.get_by_category("Clothing")[0].condition == 4.0 | |
| assert jesse.get_by_category("Clothing")[0].condition == 2.0 | |
| assert item_a in tai.inventory | |
| assert item_b in tai.inventory | |
| assert item_f in tai.inventory | |
| assert item_d in jesse.inventory | |
| assert item_e in jesse.inventory | |
| assert item_c in jesse.inventory |
| assert item_f in tai.inventory | ||
| assert item_c in jesse.inventory | ||
| assert item_c not in tai.inventory |
There was a problem hiding this comment.
I recommend checking all of the items here, rather than just the ones that were swapped.
2 participants
No description provided.