Spruce Cassandra Oh #92
Conversation
audreyandoy
left a comment
audreyandoy
left a comment
There was a problem hiding this comment.
Cassie!
Your submission completed all the waves, passed all required tests, and I loved how you experimented with writing your own test!
My main feedback focuses on utilizing guard clauses, the parent constructor, and ways to refactor. You'll notice I sounded like a broken record about guard clauses, but they really do make our code cleaner and easier to communicate our main logic for a function.
Overall, Cassie you get a 🟢 green 🟢 for this project ✨
Keep up the good work!
| class Clothing(Item): | ||
| def __init__(self, category= "Clothing", condition=float(0)): | ||
| self.category = category | ||
| self.condition = condition | ||
| def __str__(self): | ||
| return (f"The finest clothing you could wear.") | ||
|
|
There was a problem hiding this comment.
This class definition definitely works but leaves Clothing instances susceptible to having their category changed. This is where using the parent constructor is helpful to ensure that every clothing instance will have the category of 'Clothing'.
| class Clothing(Item): | |
| def __init__(self, category= "Clothing", condition=float(0)): | |
| self.category = category | |
| self.condition = condition | |
| def __str__(self): | |
| return (f"The finest clothing you could wear.") | |
| class Clothing(Item): | |
| def __init__(self, condition=0, age=0): | |
| super().__init__(category="Clothing", condition=condition, age=age) |
There was a problem hiding this comment.
Additionally, we're not truly using inheritance without the parent constructor. Inheritance is helpful so that child classes don't have to recreate the attributes, they can inherit attributes from their parent but still have unique values for them.
| class Decor(Item): | ||
| def __init__(self, category="Decor", condition=float(0)): | ||
| self.category = category | ||
| self.condition = condition | ||
| def __str__(self): | ||
| return ("Something to decorate your space.") |
There was a problem hiding this comment.
Comments from Clothing apply here.
| class Electronics(Item): | ||
| def __init__(self, category="Electronics", condition = float(0)): | ||
| self.category = category | ||
| self.condition = condition | ||
|
|
||
| def __str__(self): | ||
| return ("A gadget full of buttons and secrets.") |
There was a problem hiding this comment.
Comments from Clothing apply here as well.
| if inventory == None: | ||
| self.inventory = [] | ||
| else: | ||
| self.inventory = inventory | ||
| # if inventory != None else inventory = [] |
There was a problem hiding this comment.
Your comment had the right idea! We can refactor using a ternary:
| if inventory == None: | |
| self.inventory = [] | |
| else: | |
| self.inventory = inventory | |
| # if inventory != None else inventory = [] | |
| self.inventory = inventory if inventory is not None else [] |
| self.inventory = inventory | ||
| # if inventory != None else inventory = [] | ||
|
|
||
| def add(self, item): |
| def swap_items(self, vendor, my_item, their_item): | ||
| if my_item in self.inventory and their_item in vendor.inventory: | ||
| self.inventory.remove(my_item) | ||
| self.inventory.append(their_item) | ||
| vendor.inventory.remove(their_item) | ||
| vendor.inventory.append(my_item) | ||
| return True | ||
| else: | ||
| return False |
There was a problem hiding this comment.
Every if conditional doesn't need to be accompanied by an else. When we get into web development, else clauses are useful in scenarios where we need a 'catch all' such as handling gibberish a user enters into a form.
Let's say a user was trying to enter their email in a form.. if the user enters !@#AFS12334 into the form, we can have conditionals describe the input as needing to be a string type, containing alphanumeric values, etc. and in case the input doesn't cover all the required checks we can use else: return "Please enter a valid email" as a catch all for invalid inputs.
There was a problem hiding this comment.
We can make the appending and removing logic here more 'focused' in this function by using a guard clause:
| def swap_items(self, vendor, my_item, their_item): | |
| if my_item in self.inventory and their_item in vendor.inventory: | |
| self.inventory.remove(my_item) | |
| self.inventory.append(their_item) | |
| vendor.inventory.remove(their_item) | |
| vendor.inventory.append(my_item) | |
| return True | |
| else: | |
| return False | |
| def swap_items(self, vendor, my_item, their_item): | |
| if my_item not in self.inventory and their_item not in vendor.inventory: | |
| return False | |
| self.inventory.remove(my_item) | |
| self.inventory.append(their_item) | |
| vendor.inventory.remove(their_item) | |
| vendor.inventory.append(my_item) | |
| return True |
| def swap_first_item(self, vendor): | ||
| if len(self.inventory) > 0 and len(vendor.inventory) > 0: | ||
| self.inventory.append(vendor.inventory[0]) | ||
| vendor.inventory.append(self.inventory[0]) | ||
| self.inventory.pop(0) | ||
| vendor.inventory.pop(0) | ||
| return True | ||
| else: | ||
| return False |
There was a problem hiding this comment.
This would have been a good spot to useswap_items as a helper method as well. Regardless, keeping swap_first_item and swap_items but have a time complexity of O(n) due to the pop and remove methods. We can improve the performance of swap_first_item by using multiple assignment:
| def swap_first_item(self, vendor): | |
| if len(self.inventory) > 0 and len(vendor.inventory) > 0: | |
| self.inventory.append(vendor.inventory[0]) | |
| vendor.inventory.append(self.inventory[0]) | |
| self.inventory.pop(0) | |
| vendor.inventory.pop(0) | |
| return True | |
| else: | |
| return False | |
| def swap_first_item(self, vendor): | |
| if len(self.inventory) == 0 or len(vendor.inventory) == 0: | |
| return False | |
| self.inventory[0], vendor.inventory[0] = vendor.inventory[0], self.inventory[0] | |
| return True |
Here's a resource on time complexity of Python's methods: https://wiki.python.org/moin/TimeComplexity
Here's more info on Python multiple assignment: https://www.w3schools.com/python/gloss_python_assign_value_to_multiple_variables.asp
| else: | ||
| return False | ||
|
|
||
| def get_best_by_category(self, category): |
| def swap_best_by_category(self, other, my_priority, their_priority): | ||
| best_for_me = [] | ||
| highest_for_me = -1 | ||
| best_for_them = [] | ||
| highest_for_them = -1 | ||
| for each in self.inventory: | ||
| if each.category == their_priority and each.condition > highest_for_them: | ||
| highest_for_them = each.condition | ||
| best_for_them = each | ||
| for each in other.inventory: | ||
| if each.category == my_priority and each.condition > highest_for_me: | ||
| highest_for_me = each.condition | ||
| best_for_me = each | ||
| if best_for_me == [] or best_for_them == []: | ||
| return False | ||
| else: | ||
| self.inventory.append(best_for_me) | ||
| other.inventory.append(best_for_them) | ||
| self.inventory.remove(best_for_them) | ||
| other.inventory.remove(best_for_me) | ||
| return True No newline at end of file |
There was a problem hiding this comment.
*insert broken record about guard clauses 😅 *
Other helper methods to use in this function include: get_best_by_category and swap_items.
| @@ -0,0 +1,29 @@ | |||
| import pytest | |||
There was a problem hiding this comment.
Nice scratch file to test your condition descriptions!
|
|
||
| def condition_description(self): | ||
| if self.condition < 1: | ||
| return "You shall not pass...inspection" |
2 participants
No description provided.