Phoenix_C22_Marjana_Khatun#25
Conversation
kelsey-steven-ada
left a comment
kelsey-steven-ada
left a comment
There was a problem hiding this comment.
Looks good! Just a few pieces of feedback around things we could refactor or tests we could check some further values on. Let me know if you have questions on the feedback =]
| self.condition = condition | ||
|
|
||
| def __str__(self): | ||
| return f"An object of type Item with id {self.id}" |
There was a problem hiding this comment.
Every __str__ method for the child classes starts with a sentence of this exact form. A child class can call their parent's functions using super.
If instead of hardcoding the category Item in this string we called self.get_category(), how could we share this function with the child classes to reduce similar or repeated code in their __str__ functions?
| self.inventory= inventory | ||
| self.inventory= [] if self.inventory is None else self.inventory |
There was a problem hiding this comment.
The first line is duplicating some effort, we could remove it and just keep the second line:
self.inventory= [] if self.inventory is None else self.inventory| if self.condition == 5: | ||
| return "Excellent" | ||
| elif self.condition == 4: | ||
| return "Very Good" | ||
| elif self.condition == 3: | ||
| return "Good" | ||
| elif self.condition == 2: | ||
| return "Okay" | ||
| elif self.condition == 1: | ||
| return "Bad" |
There was a problem hiding this comment.
We don't test for this, and it isn't a requirement, but something to consider:
- In the Wave 5 description the condition can be 0-5, so what happens if the condition is a decimal rather than an integer?
- How could we update the function to handle both types?
| def get_category(self): | ||
| return "Item" |
There was a problem hiding this comment.
We could share the get_category function with the child classes as well if we did something like look up the name of the class and return it:
def get_category(self):
return self.__class__.__name__| if item not in self.inventory: | ||
| return False | ||
| else: | ||
| self.inventory.remove(item) | ||
| return item |
There was a problem hiding this comment.
We could remove the else statement without changing the outcome. This would be preferred so the main flow of the code is as un-indented and prominent as possible.
if item not in self.inventory:
return False
self.inventory.remove(item)
return item| if not self.get_by_category(category): | ||
| return None | ||
|
|
||
| best_item = self.get_by_category(category)[0] | ||
| for item in self.get_by_category(category): | ||
| if item.condition > best_item.condition: | ||
| best_item = item | ||
| return best_item |
There was a problem hiding this comment.
Nice implementation! At the time we assigned Swap Meet we didn't want folks to use higher order functions like max so they could get more practice with basics, but it's a great exercise to look at how we could use some built-in functions to reduce our code.
| if my_best_item is None or their_best_item is None: | ||
| return False | ||
|
|
||
| result = self.swap_items(other_vendor,my_best_item,their_best_item) |
There was a problem hiding this comment.
Nice code reuse with swap_items!
| result = vendor.remove(item) | ||
|
|
||
| raise Exception("Complete this test according to comments below.") | ||
| assert result == False |
There was a problem hiding this comment.
Something to consider, if the function returns False does that guarantee the function did not alter the inventory? Checking the vendor.inventory contents as well would confirm there are no unintended side effects.
- What could that look like?
| nobodys_item = Item() | ||
|
|
||
| result = fatimah.swap_items(jolie, item_b, nobodys_item) | ||
| result == False |
There was a problem hiding this comment.
Similar feedback as on the previous test, what other aspects could we confirm to ensure there were no unintended changes?
| assert result == False | ||
| assert len(tai.inventory) == 3 | ||
| assert len(jesse.inventory) == 3 | ||
| assert item_a in tai.inventory | ||
| assert item_b in tai.inventory | ||
| assert item_c in tai.inventory | ||
| assert item_d in jesse.inventory | ||
| assert item_e in jesse.inventory | ||
| assert item_f in jesse.inventory |
There was a problem hiding this comment.
Great looking assertions!
2 participants
No description provided.