Conversation
kelsey-steven-ada
left a comment
kelsey-steven-ada
left a comment
There was a problem hiding this comment.
Great work Chelsea! 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 =]
| class Clothing(Item): | ||
| def __init__(self, id=None, condition=None, fabric=None): | ||
| super().__init__(id, condition) | ||
| self.fabric = "Unknown" if fabric is None else fabric |
There was a problem hiding this comment.
Strings are an immutable type; unlike lists and dictionaries, we could set the default in the function definition to "Unknown", which could let us remove the if-check.
| def get_category(self): | ||
| return super().get_category() |
There was a problem hiding this comment.
Since this function passes along the superclass response, we could actually remove the function definition from the child class completely and python would call the superclass method by default. The same feedback applies to condition_description below.
| def __str__(self): | ||
| return f"An object of type Item with id {str(self.id)}" |
There was a problem hiding this comment.
Instead of hardcoding the class name, we could make a dynamic call to get_category:
return f"An object of type {self.get_category()} with id {str(self.id)}"This would let us reduce some repetition in the child classes since they could call the superclass __str__ to get the first sentence of the child class string descriptions.
| self.id = uuid.uuid4().int if id is None else id | ||
| self.condition = 0 if condition is None else condition |
There was a problem hiding this comment.
Nice conditional assignments!
| return self.__class__.__name__ | ||
|
|
||
| def condition_description(self): | ||
| if 0 <= self.condition <= 1: |
There was a problem hiding this comment.
I like the use of ranges to account for decimal condition values.
| my_id = int(input("Enter my id: ")) | ||
| their_id = int(input("Enter their id: ")) | ||
|
|
||
| result = self.swap_by_id(other_vendor, my_id, their_id) | ||
| return result |
There was a problem hiding this comment.
I like how you've implemented the swapping, as part of this function, we also want to print the inventory so a user can see what items are available and then choose which item they want. What code would you add to support that?
There was a problem hiding this comment.
self.display_inventory(category)
| @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.
| assert len(vendor.inventory) == 3 | ||
| assert result is None |
There was a problem hiding this comment.
Great assertions, I would also suggest checking the inventory contents, since the length being correct doesn't guarantee that the individual elements are still what we expect.
| assert not result | ||
| assert len(fatimah.inventory) == 3 | ||
| assert len(jolie.inventory) == 0 |
There was a problem hiding this comment.
The feedback in wave 1 applies here as well, what else would be helpful to assert to ensure there were no unintended side effects?
| assert item_b in tai.inventory | ||
| assert item_c in tai.inventory | ||
|
|
||
| assert len(jesse.inventory) == 0 |
2 participants
No description provided.