Conversation
passed all integration tests
All tests passed
kelsey-steven-ada
left a comment
kelsey-steven-ada
left a comment
There was a problem hiding this comment.
Great work Nad! 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 =]
| self.type = type | ||
|
|
||
| def __str__(self) -> str: | ||
| elec_str = super().__str__() |
There was a problem hiding this comment.
Really nice use of inheritance across the child classes 👍
| return str_item | ||
|
|
||
| def condition_description(self): | ||
| return self.cond_descriptions[int(self.condition)] |
There was a problem hiding this comment.
I like the use of int() to account for decimal condition values.
| if id is not None: | ||
| if type(id) is int: | ||
| self.id = id | ||
| else: | ||
| raise TypeError("The id for Item object must be an integer") | ||
| else: | ||
| self.id = uuid.uuid4().int |
There was a problem hiding this comment.
I think we could simplify the if/else tree a little by combining the id validation checks:
if id is not None and type(id) is not int:
raise TypeError("The id for Item object must be an integer")
self.id = id if id else uuid.uuid4().int| if item not in self.inventory: | ||
| self.inventory.append(item) |
There was a problem hiding this comment.
Nice check to ensure the Vendor doesn't contain duplicates
| if not item: | ||
| return item |
There was a problem hiding this comment.
This could lead to some confusion around what types this function can return. If a user accidentally input a falsy value like the integer 0, then we would be returning an integer data type here.
There was a problem hiding this comment.
Thank you for this. I'm used to do this all the time since I've have mostly worked with hard typed languages like Java or C#. I would follow your advice and return the right type or None next time.
| my_interest_item = other_vendor.get_best_by_category(my_priority) | ||
| their_interest_item = self.get_best_by_category(their_priority) |
There was a problem hiding this comment.
Loving the descriptive variable names 😊
There was a problem hiding this comment.
they are kind of a pain to type but this large names help me think better about what I'm doing xD
| print("No inventory to display.") | ||
| return | ||
|
|
||
| for i in range(len(display_items)): |
There was a problem hiding this comment.
Nice use of range to give us an index to work with! Another way we could do this is with the enumerate function:
for index, item in enumerate(display_items, start=1):
print(f"{index}. {item}")There was a problem hiding this comment.
Thank you! I love to learn other ways!
|
|
||
| clothes = nad.get_electronics_by_type("") | ||
|
|
||
| assert clothes == [] |
There was a problem hiding this comment.
Love the tests you've added! 😄
| assert len(vendor.inventory) == 3 | ||
| assert vendor.inventory == ["a", "b", "c"] | ||
| assert result is None No newline at end of file |
There was a problem hiding this comment.
Great assertions, very complete checks =]
| assert not result | ||
| assert len(fatimah.inventory) == 3 | ||
| assert len(jolie.inventory) == 0 No newline at end of file |
There was a problem hiding this comment.
Great assertions, I would also suggest checking the inventory contents, since checks like the length being correct do not guarantee that the individual elements are still what we expect.
|
Thank you Kelsey! I really appreciate all the feedback! It is a lot of work going through someone else's code |
2 participants
No description provided.