Spruce - Emily C.#74
Conversation
anselrognlie
left a comment
anselrognlie
left a comment
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. For instance, making sure your constructors only expose the parameters they need, making sure your functions meet the full specified behavior, not just passing the included tests, and making sure all the lines of code you've written do what you intended.
Overall, well done.
| class Clothing(Item): | ||
| def __init__(self, category=None, condition = 0): | ||
| super().__init__(category="Clothing", condition=condition) | ||
| self.category = "Clothing" | ||
| self.condition = condition |
There was a problem hiding this comment.
This class is properly inheriting from Item, and you made proper use of the parent class' __init__! Notice, that means that we don't need to set category and condition ourselves (since that was done when we called the parent's constructor). Also, while we need to allow our child class' constructor to be supplied a condition, we don't need (and really don't want) to have the coder calling our class constructor try to supply a category, so we can leave that off entirely. This would make the constructor look something like:
def __init__(self, condition = 0):
super().__init__(category="Clothing", condition=condition)This same simplification would apply to each of the derived classes.
| from .item import Item | ||
|
|
||
| class Decor(Item): | ||
| def __init__(self, category=None, condition = 0): |
There was a problem hiding this comment.
Nice job keeping the constructor simple here (not repeating the category and condition assignment). We can also leave off the category parameter in the constructor definition:
def __init__(self, condition = 0):| def __init__(self, category=None, condition = 0, age = 0): | ||
| self.category = category if category is not None else "" | ||
| self.condition = condition | ||
| self.age = age |
There was a problem hiding this comment.
Since you added the age optional property, you should consider adding age to the constructor parameters for the derived classes as well. We might like to make a Decor with an age of 2, for example.
And notice that if we want to use "" as the default value for the category we can do so directly. Strings are immutable, so it's safe to use them as a default value (it doesn't matter if they are shared, since they can't be mutated). The approach that you took here does let us distinguish between leaving off the category parameter (it would be set to None) and explicitly setting the value to "", so there are definitely times we might want to use this approach, but for a simple default case, again, the string can be supplied directly in the parameters.
| def condition_description(self): | ||
| if self.condition < 2: | ||
| return "This is heavily used." | ||
| if self.condition == 3-4: | ||
| return "This is in mint condition." | ||
| if self.condition >4: | ||
| return "This is in excellent condition." |
There was a problem hiding this comment.
These ranges don't cover the full set of condition values.
self.condition < 2 covers any value below 2 (but not including 2).
In self.condition == 3-4, 3-4 isn't a range. Its an expression that gets evaluated to -1. So this message would only be displayed if the condition was -1.
Then self.condition >4 covers any value above 4 (but not including 4).
So currently, any value between 2 and 4 (both inclusive) wouldn't match any output message.
We could modify the middle condition to something like 2 <= self.condition <= 4 (note this is very Python-specific. In most other languages we would need to write 2 <= self.condition and self.condition <= 4) to cover the values in the middle.
Notice the tests didn't catch this because they check that one message comes back for a condition of 5, and then any items with a condition of 1 all have the same message. Both of those conditions were covered by the logic as implemented. But consider what would have happened if the condition had been something like 3.5 (as in some of the other tests). What would condition_description have returned?
|
|
||
| class Vendor: | ||
| pass | ||
| def __init__(self, inventory=None): |
There was a problem hiding this comment.
Nice job handling the default inventory list condition.
| self.inventory.append(friend.inventory[0]) | ||
| friend.inventory.append(self.inventory[0]) | ||
| self.inventory.pop(0) | ||
| friend.inventory.pop(0) |
There was a problem hiding this comment.
Because pop has to shift the remaining items forward to fill the popped value, this is a O(n) approach. We could do this in O(1) with an approach such as:
friend.inventory[0], self.inventory[0] = self.inventory[0], friend.inventory[0]| if not items_in_category: | ||
| return None |
There was a problem hiding this comment.
If item_highest_condition were initialized to None, this check wouldn't be required.
|
|
||
| def get_best_by_category(self, category): | ||
| items_in_category = self.get_by_category(category) | ||
| item_highest_condition="" |
There was a problem hiding this comment.
Since the thing we are looking to find is an object, None would be a more appropriate initialization value. Initializing this to "" would lead me to expect the ultimate value to be a string, rather than an Item. In a dynamically-typed language like Python, using the initialization value to provide hints about what value to expect can be very helpful.
| my_items_their_priority = self.get_by_category(their_priority) | ||
| their_items_my_priority = other.get_by_category(my_priority) | ||
| if not my_items_their_priority or not their_items_my_priority: | ||
| return False | ||
| else: |
There was a problem hiding this comment.
This set of beginning checks isn't necessary. Recall that get_best_by_category (which you use later) returns None if there isn't an item of the desired category, and further recall that swap_items returns True on success and False on failure (the same as swap_best_by_category). So after retrieving the highest from the two vendors, we could simply return self.swap_items(other, my_highest, their_highest).
| their_highest = other.get_best_by_category(my_priority) | ||
|
|
||
| self.swap_items(other, my_highest, their_highest) | ||
| other.swap_items(other, my_highest, their_highest) |
There was a problem hiding this comment.
This line has no impact, and in fact will always fail (return False)! Think about why this is.
2 participants
No description provided.