Sprucie - Asya S #84
Conversation
There was a problem hiding this comment.
Asya!
Your submission has completed all the waves and passed all required tests. Your code was well-organized and utilized a lot of helper functions which made the overall code very neat!
My main feedback primarily focuses on utilizing guard clauses, avoiding mutable types as default parameters, using the parent constructor and being wary of creating unneeded instance attributes. Guard clauses can help refine our code and communicate our intentions with our functions more clearly.
Overall, this project is well-deserving of a 🟢 green 🟢 ✨
|
|
||
| ### Wave 1 | ||
|
|
||
| The first two tests in wave 1 imply: | ||
| <!-- The first two tests in wave 1 imply: |
There was a problem hiding this comment.
This README file didn't need to be pushed. We can use commands like git add to ensure that you're only adding specific files to be tracked, committed, and pushed to Github.
| if condition : | ||
| self.condition = condition | ||
| else: | ||
| self.inventory = 0 |
There was a problem hiding this comment.
The requirements don't discuss inventory for Clothing instances, so inventory can be omitted here.
| super().__init__(condition = 0, category = "") | ||
| self.category = 'Clothing' |
There was a problem hiding this comment.
Creating the category attribute here isn't necessary as we can have the parent constructor to create a category attribute for the Clothing instance by passing that value in the super() constructor.
| super().__init__(condition = 0, category = "") | |
| self.category = 'Clothing' | |
| super().__init__(condition = 0, category = "Clothing") |
| def __init__ (self, condition = 0 ): | ||
| super().__init__(condition = 0, category = "") | ||
| self.category = 'Decor' | ||
| if condition : | ||
| self.condition = condition | ||
| else: | ||
| self.inventory = 0 |
There was a problem hiding this comment.
Same comments about Clothing can also be applied for Decor.
| class Electronics(Item): | ||
| def __init__ (self, condition = 0): | ||
| super().__init__(condition = 0, category = "") | ||
| self.category = 'Electronics' | ||
| if condition : | ||
| self.condition = condition | ||
| else: | ||
| self.inventory = 0 | ||
|
|
There was a problem hiding this comment.
Same comments about Clothing can also be applied for Electronics.
| def swap_items(self, another_vendor, my_item, their_item): | ||
| if my_item not in self.inventory or their_item not in another_vendor.inventory: | ||
| return False | ||
| else: | ||
| self.inventory.remove(my_item) | ||
| another_vendor.inventory.append(my_item) | ||
| another_vendor.inventory.remove(their_item) | ||
| self.inventory.append(their_item) | ||
| return True |
There was a problem hiding this comment.
Love the guard clause!!
However, else conditions are typically used for last case scenarios. 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 refactor this function like so:
| def swap_items(self, another_vendor, my_item, their_item): | |
| if my_item not in self.inventory or their_item not in another_vendor.inventory: | |
| return False | |
| else: | |
| self.inventory.remove(my_item) | |
| another_vendor.inventory.append(my_item) | |
| another_vendor.inventory.remove(their_item) | |
| self.inventory.append(their_item) | |
| return True | |
| def swap_items(self, another_vendor, my_item, their_item): | |
| if my_item not in self.inventory or their_item not in another_vendor.inventory: | |
| return False | |
| self.inventory.remove(my_item) | |
| another_vendor.inventory.append(my_item) | |
| another_vendor.inventory.remove(their_item) | |
| self.inventory.append(their_item) | |
| return True |
| def swap_first_item(self, another_vendor): | ||
| if self.inventory == [] or another_vendor.inventory == []: | ||
| return False | ||
| else: | ||
| another_vendor.inventory.append(self.inventory[0]) | ||
| self.inventory.remove(self.inventory[0]) | ||
| self.inventory.append(another_vendor.inventory[0]) | ||
| another_vendor.inventory.remove(another_vendor.inventory[0]) | ||
|
|
||
| return True |
There was a problem hiding this comment.
See my comment above about else clauses. We could have reused the swap_items method to do the actual swapping.
| def swap_first_item(self, another_vendor): | |
| if self.inventory == [] or another_vendor.inventory == []: | |
| return False | |
| else: | |
| another_vendor.inventory.append(self.inventory[0]) | |
| self.inventory.remove(self.inventory[0]) | |
| self.inventory.append(another_vendor.inventory[0]) | |
| another_vendor.inventory.remove(another_vendor.inventory[0]) | |
| return True | |
| def swap_first_item(self, another_vendor): | |
| if self.inventory == [] or another_vendor.inventory == []: | |
| return False | |
| self.swap_items(self, another_vendor, self.inventory[0], another_vendor.inventory[0]) | |
| return True |
This method would have a worst-case time complexity of O(n) due to remove.
There was a problem hiding this comment.
We can improve the time complexity to O(1) by using multiple assignment.
self.inventory[0], friend_vendor.inventory[0] = friend_vendor.inventory[0], self.inventory[0]
Here's more info on Python multiple assignment: https://www.w3schools.com/python/gloss_python_assign_value_to_multiple_variables.asp
| def get_best_by_category(self, category): | ||
| best_condition = 0.0 | ||
| best_item = '' | ||
|
|
||
| for item in self.inventory: | ||
| if item.category == category: | ||
| if item.condition > best_condition: | ||
| best_condition = item.condition | ||
| best_item = item | ||
|
|
||
| if best_item: | ||
| return best_item | ||
| else: | ||
| return None |
There was a problem hiding this comment.
To refactor, we can assign None to best_item in the beginning. That way if the category is not found in inventory best_item will remain None.
| def get_best_by_category(self, category): | |
| best_condition = 0.0 | |
| best_item = '' | |
| for item in self.inventory: | |
| if item.category == category: | |
| if item.condition > best_condition: | |
| best_condition = item.condition | |
| best_item = item | |
| if best_item: | |
| return best_item | |
| else: | |
| return None | |
| def get_best_by_category(self, category): | |
| best_condition = 0.0 | |
| best_item = None | |
| for item in self.inventory: | |
| if item.category == category: | |
| if item.condition > best_condition: | |
| best_condition = item.condition | |
| best_item = item | |
| return best_item |
| def swap_best_by_category(self, other, their_priority, my_priority): | ||
|
|
||
| # my best item to give to them | ||
| my_best_item = self.get_best_by_category(their_priority) | ||
| # their best item to give to me | ||
| their_best_item = other.get_best_by_category(my_priority) | ||
|
|
||
| if my_best_item and their_best_item: | ||
| self.swap_items(other, my_best_item, their_best_item) | ||
| return True | ||
| else: | ||
| return False |
There was a problem hiding this comment.
Great use of helper methods!! We can also use a guard clause here!
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
No need to submit a project with this much empty space. You can use the autopep8 package to format your projects (instructions can be found in your viewing party feedback)
2 participants
No description provided.