Spruce-Sandra Caballero #75
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, 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, 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, condition = None, category = "Clothing"): | ||
| super().__init__(category, condition) |
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=None):
super().__init__(category="Clothing", condition=condition)| def __init__(self, condition = None, category = "Decor"): | ||
| super().__init__(category, condition) |
There was a problem hiding this comment.
The same situation applies here as for the Clothing constructor.
| def __init__(self, condition = None, category = "Electronics"): | ||
| super().__init__(category, condition) |
There was a problem hiding this comment.
The same situation applies here as for the Clothing constructor.
| def __init__(self, category = None, condition = None): | ||
| self.category = category if category is not None else "" | ||
| self.condition = condition if condition is not None else 0.0 |
There was a problem hiding this comment.
For these default parameters, strings and numbers are immutable, so we don't really need to go this route for setting the values. We could use "" and 0 as the default values directly.
Doing it this way can have some value since in theory it could allow us to detect the case where we are asking this constructor to use a default value (other than simply leaving off the argument). The way you structured your child class constructors benefits from this, since you can sort of "defer" picking the default value and have it only applied in the parent class. An alternative approach would be to set the default to 0 for Item and all its child classes, but then if we decided to change the defaults all to 5, we'd have to change it everywhere. With the way you set things up, you would only need to make that change here in Item.
| def condition_description(self): | ||
| if 5 >= self.condition > 4: | ||
| item_description = "New New" | ||
| elif 4 >= self.condition > 3: |
There was a problem hiding this comment.
Nice use of ranges to cover the possibility of non-integer conditions. Notice that we don't need the within range syntax (chaining relational operations) here though. If we reached this line, it means that condition wasn't greater than 4 (or it would have taken the first branch). In other words, we know that condition must at least be less than or equal to 4. So we don't need the upper part of the range and could write it simply as:
elif self.condition > 3: This is true for all of the ranges cascading down due to the order you wrote your checks!
Also, consider leaving the ranges open at the end. Yes, we can cap the score at 5 and greater than 0, but what would happen if the score happened to be "invalid"? Such as 6, or even 0 as written. What would the value of item_description be? Either explicitly handling those conditions, or loosening up some of the ranges, might be desirable.
| return items | ||
|
|
||
| def swap_items(self, other_vendor, my_item, their_item): | ||
| if my_item in self.inventory and their_item in other_vendor.inventory: |
There was a problem hiding this comment.
Here again, I would tend to reverse the sense of this comparison to check for the desired items NOT being in their respective inventories. Then we could treat this as a guard clause (returning False), and then unindenting the main logic.
And unlike the remove logic above, where we could use the error handling of the list remove method to handle the case where an item is missing, doing the explicit check works better here. If we tried to use error handling, we could end up in a situation where maybe the first remove works, but the second fails. We could write code to put things back the way they were, but instead, checking whether everything we need is in place beforehand is much easier.
| if self.inventory == [] or other_vendor.inventory == []: | ||
| return False | ||
| else: |
There was a problem hiding this comment.
It's a little more pythonic to check for an empty list by taking advantage of its falsiness rather than explicitly checking the len(), as in
if not self.inventory or not other_vendor.inventory:
return FalseAlso, notice that since the only thing you're doing in the positive condition is to return, this is essentially a guard clause. We can leave off the else and unindent the lines that were within it.
| if self.inventory == [] or other_vendor.inventory == []: | ||
| return False | ||
| else: | ||
| self.swap_items(other_vendor, self.inventory[0], other_vendor.inventory[0]) |
There was a problem hiding this comment.
Nice reuse of swap_items.
Notice, though, that by using swap_items, this winds up being O(n) (mostly due to removing the items from the inventory). We could simply swap the items in place here to have O(1) runtime with something like:
self.inventory[0], other_vendor.inventory[0] = other_vendor.inventory[0], self.inventory[0]Alternatively, we could think about how to modify the implementation of swap_items so that it would still work in general, but also give us O(1) in this specific case. Try that out if that sounds interesting!
| for item in self.inventory: | ||
| if item.category == str_category and item.condition >= rating_of_best_item: | ||
| best_item = item | ||
| rating_of_best_item = 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 best_item_for_me == None: | ||
| return False | ||
| elif best_item_for_other == None: | ||
| return False | ||
| else: | ||
| self.swap_items(other, best_item_for_other, best_item_for_me) | ||
| return True |
There was a problem hiding this comment.
Even if we don't do the explicit check for empty (None) results here, the behavior would be as expected if we called swap_items directly and returned the result. Try tracing through how this would work even if one (or both) of the best category results were None.
return self.swap_items(other, best_item_for_other, best_item_for_me)2 participants
No description provided.