sea-turtles, jae clayton #16
Conversation
There was a problem hiding this comment.
Nice job!
All required waves are complete and all 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.
The main things I would recommend focusing are are in some of the smaller details. Watch for places where your code could be a little more pythonic (the way conditions are checked, trying to use list comprehensions). Also, try looking for patterns of repeated code, and even though a helper function might not be a good fit, think about whether putting the repeated data into a collection of some kind might help us write code that's more DRY (and data-driven!).
Overall, well done.
|
|
||
|
|
||
| # with pytest.raises(TypeError): | ||
| assert result == False |
There was a problem hiding this comment.
👍
And since the description says this should literally return False, I like the explicit False check rather than a more generic falsy check.
| assert item_a != Item(category="clothing") | ||
| assert item_b != Item(category="clothing") | ||
| assert item_c != Item(category="clothing") |
There was a problem hiding this comment.
These checks aren't necessary. In fact they will always be true. One thing we haven't dwelled on too much is value equality versus reference equality. We've seen that we can compare the contents of say lists or dicts using == which does go through and compare the value of each piece of data in those collections. We can also compare to instance's ids, or use the is operator, which also compares their references (ids). For example [] == [] is true (they are both empty lists), but [] is [] is false (they are two different lists, both of which happen to be empty).
For any class we write, the default way that Python compares them with == is reference equality. So since each of those checks is comparing a previously existing instance to a newly constructed instance, all of them will compare as not equal.
We can research the __eq__ method if we'd like to provide our own value comparison logic.
| assert len(jesse.inventory) == 3 | ||
| assert len(tai.inventory) == 3 | ||
| assert result == True | ||
| assert item_f in tai.inventory | ||
| assert item_c in jesse.inventory |
There was a problem hiding this comment.
I like all of these explicit checks, as well as not making an assumption about where in the lists the swapped items will end up.
| All three classes and the Item class have an instance method | ||
| named condition_description, which should describe the condition | ||
| in words based on the value, assuming they all range from 0 to 5. | ||
| These can be basic descriptions (eg. 'mint', 'heavily used') but | ||
| feel free to have fun with these | ||
| (e.g. 'You probably want a glove for this one..."). | ||
| The one requirement is that the condition_description for | ||
| all three classes above have the same behavior. | ||
| ''' |
There was a problem hiding this comment.
I would probably list these as comments rather than a string.
|
|
||
|
|
||
| def add(self, item): |
|
|
||
|
|
||
| def __str__(self): |
| return "Hello World!" | ||
|
|
||
| def condition_description(self): |
There was a problem hiding this comment.
What would be returned if the condition were say -1, or how about 10.
Also, notice how repetitive all the conditions are. If we wanted to add additional conditions, we would need to write more code to handle them. What if we stored the conditions in a collection and could somehow calculate where in the collection the appropriate description for each condition could be found? If we do it carefully, adding additional conditions could be as simple as adding a new string to the collection, and there might be less code overall that would be easier to test (are ALL of these messages currently being tested?).
|
|
||
| class Clothing(Item): |
There was a problem hiding this comment.
👍
We definitely need to import Item so that it can be the parent class of Clothing.
| super().__init__(category= 'Clothing', condition=condition) |
There was a problem hiding this comment.
👍
Nice job removing the category from the constructor, since we don't want to encourager the caller to try to change it. Instead, we pass the literal value that we want to set up to the parent's constructor.
| return "The finest clothing you could wear." | ||
|
|
||
|
|
There was a problem hiding this comment.
Notice that we don't need to define condition_description. Since we want the same implementation for this child class as for the parent class, by leaving it out of the class definition, we will inherit the method from the parent.
No description provided.