C18 Cheetahs - Sarah Sanborn #25
Conversation
…for get_by_category from string to empty list.
…line to come before item from self inventory was removed.
…ll as a stringify method. Uncomment first test of Wave 5.
…s as well as stringify method.
…lectronics.py. Also added stringify method to same lass. Uncommented test 21.
…removed or commented out section to come back to it later.
…ded super() to each dunder init of those classes. Wave 5.
…hing, Decor, and Electronic classes to use the condition_description instance method within Item.
…of my Vendor class.
…ng max() and lambda.
…Also added code if no item within category, return None.
…y_category test. Implemented swap_best_by_category in Vendor class.
…p decorator and raise exception decorator.
…ithout needing to update code.
…ut updating code.
…y_category_no_match_is_false.
…length is zero rather than an empty list.
There was a problem hiding this comment.
Hi Sarah! Your project has been scored as yellow due to improper inheritance. You can see more detailed comments about this in your code, but I would highly recommend reviewing how Inheritance works and when to use super().
Great job making frequent commits and have descriptive commit messages! ✨
There are several methods in your Vendor class where a try-except block could be implemented to improve time complexity. Checking to see if an item is in a list before doing something with it, is an O(n) operation. So, see if it's possible to replace those if statements with a try-except block instead. :)
Nice job overall! You can find my comments in your code. Let me know if you have any questions. 😊
| # ********************************************************************* | ||
| # ****** Complete Assert Portion of this test ********** | ||
| # ********************************************************************* | ||
| # Assertions should check: | ||
| # - That the results is truthy | ||
| # - That tai and jesse's inventories are the correct length | ||
| # - That all the correct items are in tai and jesse's inventories, including the items which were swapped from one vendor to the other | ||
| assert result == True |
| def __init__(self, category = None, condition = 0): | ||
| if category is None: | ||
| category = "" |
There was a problem hiding this comment.
Strings are an immutable data type so it's safe to set them as a default value in your constructor :)
| return "Well, at least it's free" | ||
| elif self.condition >= 1: | ||
| return "Better than a poke in the eye with a sharp stick" |
There was a problem hiding this comment.
Lol love this condition 😆
Nice job with your Item class!
| super().__init__(category = None, condition = 0) | ||
| self.category = category | ||
| self.condition = condition |
There was a problem hiding this comment.
This constructor works but it's actually doing more work than is necessary.
Let's say a user calls your constructor and creates a new instance of the clothing class like this:
shirt = Clothing("shirt", 3)
The function now has the following variables: category = "shirt" and condition = 3
The next thing that happens is
super().__init__(category=None, condition=0)
This line calls super() which in this case is the Item class. And then it calls the Item class's __init__() method. Doing super().__init__() specifies that we want to take advantage of inheritance by using the code already written in the parent class here. So, under the hood, Python is going to grab that code, but run it here in the context of this class. So, it's going to pass category = None and condition = 0 into the function which will update shirt's attributes to be:
shirt.category = None
shirt.condition = 0
Next we have lines 7 & 8 which update shirt's attributes again, but this time with the arguments passed into the Clothing constructor and now stored in the category and condition variables. This gives us:
shirt.category = "shirt"
shirt.category = 3
To prevent your code from updating the attributes twice, we should either call super().__init__() and pass the variables in there or declare the attributes and bind them locally like you did on lines 7 and 8. I would recommend using super().__init__() because it ensures that attributes of Clothing and Item are being handled the same way.
Another thing to note is that I was able to update the category to be "shirt" even though it should have a category of "Clothing". To prevent changes to the Clothing class's category, you can remove category from the Clothing constructor's parameters and hardcode the value instead.
So, you could refactor this constructor to be:
def __init__(self, condition=0):
super().__init__("Clothing", condition)
Now, users won't be able to change the category and the attributes will only be updated once. 😄
|
|
||
| def remove(self, item): | ||
| if item not in self.inventory: |
There was a problem hiding this comment.
This approach works, but consider how a try-except block could improve your time complexity. 🤔 💭 😌
|
|
||
| def get_by_category(self, category): | ||
| items_list = [item for item in self.inventory if item.category == category] |
| items_list = [item for item in self.inventory if item.category == category] | ||
|
|
||
| if items_list == []: |
There was a problem hiding this comment.
Consider if this conditional is necessary. What would happen if this function only had the list comprehension and return items_list 🤔 💭 😌
Also! this is a small note but it's more Pythonic to write if items_list: or if not items_list: since lists are truthy/falsey
| return False | ||
|
|
||
| # I hard-coded this, but was curious about how this could be scaled-up. |
There was a problem hiding this comment.
Not sure what you mean by "hard coded". That usually refers to having hard coded data where it would be better to use a variable or something that would leave room for different values.
That said, this function looks great! You could probably use a try-except here too to make this more efficient though.
| if self.get_by_category(category) == []: | ||
| return None | ||
| best_item = max(self.get_by_category(category), key = lambda i : i.condition) |
There was a problem hiding this comment.
Ooo! Great usage of the max() function with a lambda function! 🤩
No description provided.