Zoisite Katherine G and Say R#79
Conversation
resolving merge conflict
kelsey-steven-ada
left a comment
kelsey-steven-ada
left a comment
There was a problem hiding this comment.
Great work! I've left a mix of suggestions and questions to consider for feedback. Please reply here on Github or reach out on Slack if there's anything I can clarify =]
| @pytest.mark.skip | ||
| @pytest.mark.integration_test | ||
| # @pytest.mark.skip | ||
| # @pytest.mark.integration_test |
There was a problem hiding this comment.
For integration tests, we want to leave the decorator @pytest.mark.integration_test uncommented. This decorator tells pytest that this is an integration test, so we should run it after all of the unit tests.
| # ********************************************************************* | ||
| # ****** Complete Assert Portion of this test ********** | ||
| # ********************************************************************* | ||
| assert result == False No newline at end of file |
There was a problem hiding this comment.
Something to consider, if the function returns False does that guarantee the function did not alter the inventory? Checking the vendor.inventory contents as well would confirm there are no unintended side effects.
- What could that look like?
- Are there tests in other waves that could benefit from similar changes?
| assert len(tai.inventory) == 3 | ||
| assert len(jesse.inventory) == 3 |
There was a problem hiding this comment.
I highly suggest checking all the items in the lists in these tests to ensure there were no unintended side effects. What might that look like?
| class Vendor: | ||
| pass No newline at end of file | ||
| def __init__(self, inventory = None): | ||
| self.inventory = [] if inventory is None else inventory |
There was a problem hiding this comment.
Great use for a ternary operator!
| if item not in self.inventory: | ||
| return False | ||
| else: | ||
| self.inventory.remove(item) | ||
| return item |
There was a problem hiding this comment.
The else keyword doesn't change the outcome of the code in this case, I suggest removing it so the code in that block can be un-indented:
if item not in self.inventory:
return False
self.inventory.remove(item)
return item| if self.condition >= 0.0 and self.condition < 1.0: | ||
| return "Should you get this? IT DEPENDS! ʕ •ᴥ• ʔ" | ||
| elif self.condition >= 1.0 and self.condition < 2.0: | ||
| return "One person's garbage is another's treasure." | ||
| elif self.condition >= 2.0 and self.condition < 3.0: | ||
| return "If you squint, you can't even tell." | ||
| elif self.condition >= 3.0 and self.condition < 4.0: | ||
| return "This is a bargan for the price." | ||
| elif self.condition >= 4.0 and self.condition < 5.0: | ||
| return "This item has been well cared for, but you can tell you're not the first owner." | ||
| elif self.condition >= 5.0: | ||
| return "This item is immaculate. If you don't buy it, I will!" |
There was a problem hiding this comment.
Another approach for finding the condition description could be to make a dictionary that maps conditions as keys to descriptions as values. There's a trade off of using more memory creating the map variable, but if memory isn't a concern, for me, it's a little easier to read.
condition_map = {
0: "Not Usable”,
1: "Gross",
2: "Wear and Tear",
3: "Not Great",
4: "Gently used",
5: "Like new"
}
return condition_map[int(self.condition)]| if matching_items: | ||
| return matching_items | ||
| else: | ||
| return [] |
There was a problem hiding this comment.
I see an opportunity to reduce the code a little, do we need to create a new empty list on line 51 if matching_items is empty?
|
|
||
| def get_by_category(self, category): | ||
| matching_items = [] | ||
| matching_items = [item for item in self.inventory if item.get_category() == category] |
There was a problem hiding this comment.
Great place for a list comprehension!
| items = self.get_by_category(category) | ||
| if not items: | ||
| return None | ||
| return max(items, key=lambda item: item.condition) |
| def swap_best_by_category(self, other_vendor, my_priority, their_priority): | ||
| my_best_item = self.get_best_by_category(their_priority) | ||
| their_best_item = other_vendor.get_best_by_category(my_priority) | ||
| if my_best_item and their_best_item: |
There was a problem hiding this comment.
Could we change this into a guard statement to let us unindent the intended flow of the function?
3 participants
No description provided.