Spruce - Reid #83
Spruce - Reid #83
Conversation
…creating class Item.
…s of Item in clothing.py.
…ctronics, which are child classes of Item.
…lled condition_description() which returns the condition as an str.
…n Vendor, but it is not behaving as expected.
There was a problem hiding this comment.
Nice job!
All required waves are complete and all required tests are passing! Your code is well organized, and you are generally using good, descriptive variable names. Your general approaches are good, and you're off to a good start with OOP.
Be a little careful of where certain instance members "live". For instance, if the condition member is defined only in the child classes of Item, then we shouldn't try to access it within Item itself. If we want to do that (which made sense in this project), then we should move the condition member itself up to the parent class Item.
Also, be sure to be reading the tests carefully to understand what they're testing, as well as what they're not. Be sure to think about other situations that our code might need to handle (especially situations that show up in other tests, but which might not themselves be tested). The supplied tests are an important baseline, but we should still think about the overall behavior of our code and how it handles a variety of situations.
Also, we can continue to work on making our python code more idiomatic. Avoiding unnecessary indenting (by using guard clauses), and checking conditions making use of pythons ideas about truthy and falsy can communicate your intent to other python developers very quickly (even if it might not be so clear to new developers).
Overall, well done.
| def __init__(self, category = ""): | ||
| self.category = category |
There was a problem hiding this comment.
Notice this requirement in the project description:
All three classes and the Item class have an attribute called condition, which can be optionally provided in the initializer. The default value should be 0.
Rather than each of the child classes defining it condition on its own, we can add the condition to the Item constructor, like
def __init__(self, category="", condition=0):
self.category = category
self.condition = condition| def __str__(self): | ||
| return "Hello World!" | ||
|
|
||
| def condition_description(self): |
There was a problem hiding this comment.
Good job putting this implementation in the parent class, but also notice that this method is expecting the instance to have an instance member called condition, but currently Item doesn't have that. So if we created an Item and then tried calling its condition_description method, Python would crash with an AttributeError. The child classes derived from Item do have this attribute (although, think about what would happen if we forgot to in one), but in general we shouldn't make assumptions about a class having attributes that aren't defined in the class itself (although this can be used for a pattern called the Template Method pattern). So trying to access the condition method here in Item should also give us a hint that initializing the condition in our constructor is a good idea.
| return "Ew." | ||
| elif self.condition == 2: | ||
| return "Um." | ||
| elif self.condition == 3: |
There was a problem hiding this comment.
What would happen if we tried to get the description for an Item with a condition of 3.5 (there are tests that set the condition to 3.5, but don't try to read the description). The description test checks for a condition of 1 and a description of 5, but we should still make sure our code covers any other reasonable conditions, including floating point values (since we've seen them used in other tests).
How could we modify this (slightly) to cover the values between the whole numbers?
| super().__init__(category = "Clothing") | ||
| self.condition = condition |
There was a problem hiding this comment.
If we added support for the condition attribute to Item itself (and updated the constructor to match, see below) then we could make use of the constructor argument to pass through the condition parameter here.
super().__init__(category="Clothing", condition=condition)Also, good call on removing category from the Clothing constructor. We don't want to allow the creation of a Clothing instance for which the category isn't "Clothing". (Of course, since this is Python, we could simply reassign it later on. But there are other Python features that could help us prevent that if we really needed to!)
| super().__init__(category = "Decor") | ||
| self.condition = condition |
There was a problem hiding this comment.
The same situation applies here as for the Clothing constructor.
| for item in self.inventory: | ||
| if ((item.category == category) and | ||
| (item.condition > best_condition)): | ||
| best_item = item | ||
| best_condition = item.condition |
There was a problem hiding this comment.
This logic is doing two things: 1. restricting to items of a certain category, and 2. looking for the best condition. Do we have a method that filters to a category that we could reuse here?
| if not self_best or not other_best: | ||
| return False | ||
| else: | ||
| self.swap_items(other, self_best, other_best) | ||
| return True |
There was a problem hiding this comment.
Because of how swap_items is implemented, the empty checks here aren't necessary. So long as we make use of the return value from swap_items, we could get away with
return self.swap_items(other, self_best, other_best)|
|
||
| return True | ||
|
|
||
| def get_best_by_category(self, category = ""): |
There was a problem hiding this comment.
The description of this function didn't ask for a default value for the category parameter. And arguably, it makes sense to require the caller to provide a value. Since the function is called get_best_by_category, I would expect to need to have a particular category in mind, and it might read a little strangely to see a call like some_vendor.get_best_by_category() where the category isn't stated.
| best_condition = item.condition | ||
| return best_item | ||
|
|
||
| def swap_best_by_category(self, other = None, my_priority = "", their_priority = ""): |
There was a problem hiding this comment.
The description of this function didn't ask for a default value for the parameters. And in cases where a default is supplied, it's a little surprising that it can lead to an error. Notice that if None is used for other, line 62 will crash when it tries to access get_best_by_category for a NoneType. If instead we lead off the default values, then if we tried to call the method without the needed parameters we would get an error about some required parameters being missing.
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Mostly a nit pick, but try to not leave big chunks of unnecessary whitespace in your files.
No description provided.