Spruce- Ivette F. #82
Conversation
…and completed wave 3 passing all tests
…tem-child classes
…and swap_by_newest() as instance methods to Vendor class; no tests for new methods but still passing all wave tests
… remove instanc methods
There was a problem hiding this comment.
Nice job!
All required waves are complete and all required tests (and then some) are passing! Your code is well organized, and you are generally using good, descriptive variable names. Your general approaches are good, you're off to a good start with OOP, and you're making good reuse of functions.
The main things I would recommend focusing are are in some of the smaller details. For instance, notice that none of the Item child classes have tests that need a category parameter. And in fact, we would probably want to prevent a user from creating an Electronics with a category of "Decor", so really consider which constructor parameters we need to provide.
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.
And nice job adding the age property and swap method! Adding the helper method worked really well, and also made it easier to add more targeted unit tests.
Well done!
| def __init__(self, category="Clothing", condition=0, age=None): | ||
| super().__init__(category, condition, age) |
There was a problem hiding this comment.
We don't really want to let users of our class change the category for a Clothing instance to something else (at least not from the constructor). We can leave the category out of the constructor parameters entirely, and pass the desired category literally in to the parent's constructor as follows:
def __init__(self, condition = 0, age = None):
super().__init__("Clothing", condition, age)| def __init__(self, category="Decor", condition=0, age=None): | ||
| super().__init__(category, condition, age) |
There was a problem hiding this comment.
The same situation applies here as for the Clothing constructor.
| def __init__(self, category="Electronics", condition=0, age=None): | ||
| super().__init__(category, condition, age) |
There was a problem hiding this comment.
The same situation applies here as for the Clothing constructor.
| @@ -1,2 +1,23 @@ | |||
| class Item: | |||
| pass | |||
| def __init__(self, category="", condition=0, age=None): | |||
There was a problem hiding this comment.
I was wondering why you decided to use None as the default for age but I see that it looks like you decided to exclude any items from the age-related extension features unless it was explicitly set. Neat decision!
| def __str__(self): | ||
| return "Hello World!" | ||
|
|
||
| def condition_description(self): |
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?
| if item.age != None: | ||
| only_ages.append(item) | ||
| if only_ages: | ||
| newest_item = min(only_ages, key = lambda item : item.age) |
There was a problem hiding this comment.
Like max, min also supports a default parameter. How would that let us simplify this?
| """ | ||
|
|
||
| """ | ||
| my_newest = self.get_by_newest() |
There was a problem hiding this comment.
Nice use of the extra helper function you added!
| if my_newest and their_newest: | ||
| self.swap_items(other, my_newest, their_newest) | ||
| return True | ||
| else: | ||
| return False |
There was a problem hiding this comment.
Notice how similar this is to swap_best_by_category. Knowing how swap_items operates, how could we simplify this part?
| from swap_meet.decor import Decor | ||
| from swap_meet.electronics import Electronics | ||
|
|
||
| def test_get_by_newest(): |
There was a problem hiding this comment.
Nice job adding tests for your age-related functions. One thing I'd be a little careful of is that for most of these tests, the newest item happens to be the first thing in the inventory. Try having the newest at a more varied distribution of locations. Until the very final test, I think most of these tests would pass if get_newest_item simply returned the first thing in the inventory.
| assert newest_item.category == "Clothing" | ||
| assert newest_item.age == pytest.approx(2.0) | ||
|
|
||
| def test_get_by_newest_some_items_with_None_age(): |
There was a problem hiding this comment.
This test doesn't really prove to me that this is ignoring the None values. Again, from this, it could just be that the first item is being returned immediately and the items with None for their age aren't even being examined. We could make this a more robust test by moving the newest item so that it came between to None records (it would then be reasonable that the code had to skip over at least one of them). Or have a test where we try to get the newest item from a list containing only items with None ages, which should give us None as a result.
No description provided.