Scissors! ~ Jittania #64
Conversation
There was a problem hiding this comment.
Great work on your first OOP project. You've met the learning goals around reading test and using composition and inheritance. I am particularly impressed by the way you've spelled out the requirements. I've left a few comments on ways you could consider refactoring focused on using super() and using helper methods. Please let me know if you have any questions. Keep up the hard work!
| @@ -0,0 +1,129 @@ | |||
| # Wave 1 | |||
There was a problem hiding this comment.
Great work writing down all the requirements!
| def __init__(self, category="Clothing", condition=0): | ||
| self.category = category | ||
| self.condition = condition |
There was a problem hiding this comment.
Consider using super. Category doesn't ever need to be passed in when instantiating a Clothing object since it will always be "clothing"
| def __init__(self, category="Clothing", condition=0): | |
| self.category = category | |
| self.condition = condition | |
| def __init__(self, condition=0): | |
| super().__init__(category="Clothing", condition=condition) |
| if my_item not in self.inventory or their_item not in friend.inventory: | ||
| return False | ||
|
|
||
| # take my_item out of my inventory list and adds it to friend's inventory |
There was a problem hiding this comment.
Good use of add and remove as help functions.
| def swap_items(self, friend, my_item, their_item): | ||
|
|
||
| # guard clause (method cannot find my_item or their_item in either inventory list and/or inventories are empty) | ||
| if my_item not in self.inventory or their_item not in friend.inventory: |
There was a problem hiding this comment.
Nice work dealing with the edge case first.
| # removes first item in `inventory` of `friend` and adds to `inventory` of recipient Vendor | ||
| friend_first_item = friend.inventory[0] | ||
|
|
||
| friend.remove(friend_first_item) | ||
| self.add(friend_first_item) | ||
|
|
||
| # removes first item in `inventory` of recipient Vendor and adds to `inventory` of `friend` | ||
| self_first_item = self.inventory[0] | ||
| self.remove(self_first_item) | ||
| friend.add(self_first_item) |
There was a problem hiding this comment.
Consider refactoring to use swap_items
| if best_item == 0: | ||
| return None |
There was a problem hiding this comment.
Consider how if you initiated best_item = None on line 69, lines 80 and 81 wouldn't be necessary.
| for my_item in self.inventory: | ||
| if my_item.category == their_priority: | ||
| other.add(my_item) | ||
| self.remove(my_item) | ||
|
|
||
| for other_item in other.inventory: | ||
| if other_item.category == my_priority: | ||
| self.add(other_item) | ||
| other.remove(other_item) |
There was a problem hiding this comment.
hrmmm, I'm not sure that this will in fact swap the best of a particular category. It looks like it will swap the first item in the inventory that matches the priorities. This means that our tests aren't thorough enough! To make sure that you're getting the best item of particular category, use get_best_by_category and call it on self and other. You can then use swap_items to swap the items you identify.
No description provided.