project complete #90
project complete #90
Conversation
There was a problem hiding this comment.
Great work on your first OOP project. It is clear that the learning goals around defining classes with instance variables and methods and using inheritance were met. Your code is clear and logical, and you've made good use of helper functions. I've left a few comments on ways you might consider refactoring. In particular, I've noted how you could use a pattern of checking for an edge case and then moving the main logic of the function outside a conditional to enhance readability. With regards to git, it would be good to practice making commits throughout the project. You can use a commit after completing each wave as a rule of thumb. When you submit the project, name your pull request "Cedar- Rae". Overall, nice work!
|
|
||
| class Decor(Item): | ||
| def __init__(self, condition = 0): | ||
| super().__init__(category = "Decor", condition = condition) |
|
|
||
| def condition_description(self): | ||
| condition_statements = ["Terrible","Bad","OK","Good","Excellent"] | ||
| return condition_statements[self.condition-1] |
There was a problem hiding this comment.
Per our discussion in class, you can use round or int for this method to work for float values of condition. Otherwise, nice appraoch!
| if not self.remove(my_item): | ||
| return False | ||
| if not friend.remove(their_item): | ||
| self.add(my_item) | ||
| return False |
There was a problem hiding this comment.
Consider including this in one compound conditional to enhance readability.
| if not self.remove(my_item): | |
| return False | |
| if not friend.remove(their_item): | |
| self.add(my_item) | |
| return False | |
| if not self.remove(my_item) or not friend.remove(their_item): | |
| return False |
| result = item | ||
| if item in self.inventory: | ||
| self.inventory.remove(result) | ||
| else: | ||
| return False | ||
| return result |
There was a problem hiding this comment.
Consider first testing for the edge case, and then moving the main functionality of your function outside the if/else. This programming pattern is called a guard clause and can among other things enhance the readability of your code.
| result = item | |
| if item in self.inventory: | |
| self.inventory.remove(result) | |
| else: | |
| return False | |
| return result | |
| if item not in self.inventory: | |
| return False | |
| self.inventory.remove(result) | |
| return item |
In addition, assigning result = item means we need to take a step to understand what result is. Consider simply returning item
| return None | ||
| max_value = 0 | ||
| max_item_value = None | ||
| for item in items: |
There was a problem hiding this comment.
Great work implementing this max algorithm
| if my_swap and their_swap: | ||
| self.swap_items(other,my_swap,their_swap) | ||
| return True | ||
| return False |
There was a problem hiding this comment.
For consistency, and to remove logic from an ident, consider refactoring in the following way:
| if my_swap and their_swap: | |
| self.swap_items(other,my_swap,their_swap) | |
| return True | |
| return False | |
| if my_swap and their_swap: | |
| return False | |
| self.swap_items(other,my_swap,their_swap) | |
| return True |
| max_value = 0 | ||
| max_item_value = None | ||
| for item in items: | ||
| if item.condition > max_value: | ||
| max_value = item.condition | ||
| max_item_value = item |
There was a problem hiding this comment.
| max_value = 0 | |
| max_item_value = None | |
| for item in items: | |
| if item.condition > max_value: | |
| max_value = item.condition | |
| max_item_value = item | |
| max_item = items[0] | |
| for item in items: | |
| if item.condition > max_item.condition: | |
| max_item = item |
No description provided.