Scissors - Stella #63
Conversation
There was a problem hiding this comment.
Great work on your first OOP project! You've met the learning goals around reading (and writing!) tests and using composition and inheritance. Your code is logical and readable, and you've made great use of helper functions. I've left a few comments on small ways you could consider refactoring, and ideas on a few additional tests/assertions. Keep up the hard work!
|
|
||
| #Wave 5: | ||
| class Decor(Item): | ||
| def __init__(self,condition = 0, age =0): |
| if item in self.inventory: | ||
| self.inventory.remove(item) | ||
| return item | ||
| else: | ||
| return False |
There was a problem hiding this comment.
Consider using the pattern where you check the edge case first:
| if item in self.inventory: | |
| self.inventory.remove(item) | |
| return item | |
| else: | |
| return False | |
| if item not in self.inventory: | |
| return False | |
| self.inventory.remove(item) | |
| return item |
| def swap_items(self,friend, my_item, their_item): | ||
| #if len(self.inventory) == 0 or len(friend.inventory) ==0: | ||
| #return False | ||
| if my_item in self.inventory and their_item in friend.inventory: |
There was a problem hiding this comment.
Consider refactoring so you check the condition where you return False first, and then move the main logic out of the conditional.
| if len(self.inventory) == 0 or len(friend.inventory) ==0: | ||
| return False | ||
| else: | ||
| return self.swap_items(friend,self.inventory[0],friend.inventory[0]) |
There was a problem hiding this comment.
You can move this line our of the else to enhance readability.
| best_condition = item.condition | ||
| return best_item | ||
|
|
||
| def swap_best_by_category(self,other,my_priority,their_priority): |
There was a problem hiding this comment.
Wow! So efficient. Good use of the previous methods as helper functions.
|
|
||
|
|
||
| #Wave 7 - Optional Enhancement | ||
| #Helper function - get item with the min age |
There was a problem hiding this comment.
Great work implementing this optional part.
| jesse = Vendor(inventory=[item_d, item_e]) | ||
| result = tai.swap_by_newest(jesse) | ||
|
|
||
| assert result is True |
There was a problem hiding this comment.
Good work writing your own tests! It's good practice to add a few more assertions here to make sure that the swap actually happened. Check that tai now has item_d and jesse has item_a (did I get that right?). I would also add an edge case test. What happens when the vendors have no inventory? What happens when all the items in their inventory have the same age?
No description provided.