Conversation
kelsey-steven-ada
left a comment
kelsey-steven-ada
left a comment
There was a problem hiding this comment.
Great work Monica! 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 =]
| 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.
There was a problem hiding this comment.
Oh yes! That's true. I will fix this issue.
| return super().get_category() | ||
|
|
||
| def __str__(self): | ||
| return f"{super().__str__()}. This is a {self.type} device." |
There was a problem hiding this comment.
Really nice use of inheritance in the __init__ and __str__ functions across the child classes.
| return type(self).__name__ | ||
|
|
||
| def condition_description(self): | ||
| if self.condition == 0: |
There was a problem hiding this comment.
Nice handling for condition description! It looks like the code assumes that self.condition will always be an integer, but what if we allowed decimals like a condition of 3.5? How could we adapt our code to convert the decimal to an int we could use here, or alternately, change the if-statements to support decimals?
| if item in self.inventory: | ||
| raise ValueError( | ||
| "Item already exist in the current inventory, item has not been added") |
There was a problem hiding this comment.
Nice check to ensure the Vendor doesn't contain duplicates
| for item in self.inventory: | ||
| if item.id == item_id: | ||
| return item | ||
| return None # Return None if no matching item |
There was a problem hiding this comment.
I like the explicit return for None when the item can't be found
| else: | ||
| index = 1 | ||
| for item in items_in_category: | ||
| print(f"{index}. {item.__str__()}") |
There was a problem hiding this comment.
Because we have implemented the __str__ method, this is one of the situations where python will implicitly call it for us, we don't need to manually invoke it inside the f-string:
print(f"{index}. {item}")| # ********************************************************************* | ||
| # ****** Complete Assert Portion of this test ********** | ||
| # ********************************************************************* | ||
| assert result is None |
There was a problem hiding this comment.
Nice assert, we definitely want to confirm our return value is None. We generally also want to confirm that there weren't side affects or changes to the input that we didn't intend. What could we check about the vendor's inventory to confirm that its content didn’t change?
There was a problem hiding this comment.
We could confirm the size of the vendor's inventory to make sure it didn't get changed.
(eg. assert len(vendor.inventory == 3)
We could also check if all items we originally added to the vendor's inventory still exist after the function call.
(eg. assert "a" in vendor.inventory
assert "b" in vendor.inventory
assert "c" in vendor.inventory)
There was a problem hiding this comment.
Nice, those would be great to add!
| # ****** Complete Assert Portion of this test ********** | ||
| # ********************************************************************* | ||
|
|
||
| assert result is False |
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_a in tai.inventory | ||
| assert item_b in tai.inventory | ||
| assert item_d in jesse.inventory | ||
| assert item_e in jesse.inventory |
There was a problem hiding this comment.
Really nice asserts, I would also suggest checking item_c in jesse.inventory
| assert item_a in their_vendor.inventory | ||
| assert item_c in my_vendor.inventory | ||
| assert len(their_vendor.inventory) == 2 | ||
| assert len(my_vendor.inventory) == 2 No newline at end of file |
| def condition_description(self): | ||
| if self.condition == 0: | ||
| # Round up condition if it is not an integer | ||
| condition = math.floor(self.condition) |
There was a problem hiding this comment.
Great use of floor for handling decimals.
3 participants
Acc 2 Swap Meet Project