Conversation
kelsey-steven-ada
left a comment
kelsey-steven-ada
left a comment
There was a problem hiding this comment.
Great work Kelly! 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 =]
| super().__init__(id, condition) | ||
| self.fabric = fabric | ||
|
|
||
| def get_attribute(self): |
There was a problem hiding this comment.
I fully see what you were doing with get_attribute function, and it is a really neat solution to the issue. However, I think we're bending the guidelines of OOP design with how it's implemented. Name-wise, another developer unfamiliar with the project might have some confusion around what determines what of the several attributes the class holds is returned by a function get_attribute. If it's a function all child classes have, generally we would define it in the parent class so the children could inherit and override as necessary. I'd need to think a bit more about if there is a specific pattern that would work better, but one suggestion would be to give the parent class a function like get_comparison_key that each of the children can override.
| fabric attribute. | ||
| """ | ||
|
|
||
| def __init__(self, fabric = "Unknown", id = None, condition = 0): |
| if id is not None: | ||
| if type(id) is not int: | ||
| raise ValueError("invalid id input") | ||
| else: | ||
| self.id = id | ||
| else: | ||
| self.id = uuid.uuid1().int |
There was a problem hiding this comment.
I think we could simplify the if/else tree a little by combining our validation checks:
if id is not None and type(id) is not int:
raise ValueError("invalid id input")
self.id = id if id else uuid.uuid1().int| 4 : "not bad", | ||
| 5 : "excellent" | ||
| } | ||
| return switch.get(self.condition) |
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?
| def get_by_id(self, id): | ||
| """Gets item by item id from list of inventory. Returns Item object.""" | ||
|
|
||
| item = next((item for item in self.inventory if item.id == id), None) |
There was a problem hiding this comment.
Nice use of next with a None default!
| items = self.get_by_category(category) if category else self.inventory | ||
| if items: | ||
| for i, item in enumerate(items, 1): | ||
| print(f"{i}. {str(item)}") |
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 wrap item in a call to str:
print(f"{i}. {item}")|
|
||
| items = self.get_by_category(category) if category else self.inventory | ||
| if items: | ||
| for i, item in enumerate(items, 1): |
| assert len(vendor.inventory) == 3 | ||
| assert item not in vendor.inventory | ||
| assert result is None |
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.
| assert len(fatimah.inventory) == 3 | ||
| assert len(jolie.inventory) == 0 | ||
| assert not result |
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?
| items = vendor.get_by_category_attribute("Electronics", "radio") | ||
|
|
||
| assert items == [] | ||
| assert len(items) == 0 No newline at end of file |
There was a problem hiding this comment.
Very nice, love the new tests =]
2 participants
No description provided.