Sapphire - Ericka M.#87
Conversation
tildeee
left a comment
tildeee
left a comment
There was a problem hiding this comment.
First of all, I want to say that I am so sorry for such a late review on this project. I hope that this feedback is still helpful!
Great work on this project, Ericka! This project is marked as a "green" on my end.
Overall, your code looks great! I see you practicing instance methods, OOP, inheritance, and testing. There are a few more opportunities and suggestions I have on instance methods and testing that I hope you can take to future projects, but please please please let me know if you have any questions on it.
In addition, your git hygiene is pretty good. Keep it up. As personal preference, I prefer commit messages that describe what code changes are in the commit ("implements stringify for item" "refactors swapping" etc) rather than "wave 4 complete" etc.
That being said, keep it up! Let me know any questions I can answer or clarify, great wrok!
| self.inventory.remove(my_item) | ||
| other_vendor.inventory.append(my_item) | ||
| other_vendor.inventory.remove(their_item) | ||
| self.inventory.append(their_item) |
There was a problem hiding this comment.
This is a good opportunity to use the other Vendor instance methods you defined! Consider this code, which passes the tests:
self.remove(my_item)
other_vendor.add(my_item)
other_vendor.remove(their_item)
self.add(their_item)| else: | ||
| my_item = self.inventory[0] | ||
| their_item = other_vendor.inventory[0] | ||
| self.swap_items(other_vendor, my_item, their_item) |
| return True | ||
|
|
||
| def get_by_category(self, category): | ||
| self.category = category |
There was a problem hiding this comment.
Here, you're assigning a new instance variable named category (therefore self.category). This line is saying "for every instance of Vendor, it will have a category that we can access using instance_of_vendor.category". However, in this case, we don't need every instance of Vendor to hold a category. You can delete this line and it'll still do the same thing and the tests still pass!
|
|
||
|
|
||
| def get_best_by_category(self, category): | ||
| self.category = category |
There was a problem hiding this comment.
Similar to above, we don't need to assign a new instance variable category onto Vendor. We can delete this line! Let me know if you have questions on this.
| best_condition = 0.0 | ||
| best_item = None | ||
| for item in self.inventory: | ||
| if category == item.get_category(): | ||
| if item.condition >= best_condition: | ||
| best_item = item | ||
| best_condition = item.condition |
There was a problem hiding this comment.
This solution is a good one because it's efficient and only goes through the self.inventory list once. However, just to make sure you know an alternative, you could consider using get_by_category first, which might help this function be not so nested:
best_condition = 0.0
best_item = None
for item in self.get_by_category(category):
if item.condition >= best_condition:
best_item = item
best_condition = item.conditionThis still passes the tests
| for_them = self.get_best_by_category(their_priority) | ||
| other_vendor.inventory.append(for_them) | ||
| self.inventory.remove(for_them) | ||
| for_me = other_vendor.get_best_by_category(my_priority) | ||
| self.inventory.append(for_me) | ||
| other_vendor.inventory.remove(for_me) |
There was a problem hiding this comment.
These lines look similar to the implementations of other instance methods... add, remove, and even swap_items! Imagine:
for_them = self.get_best_by_category(their_priority)
for_me = other_vendor.get_best_by_category(my_priority)
self.swap_items(other_vendor, for_them, for_me)| assert result == True | ||
| assert len(jesse.inventory) == 3 |
There was a problem hiding this comment.
Imagine if your function swap_best_by_category had a bug where it swapped two items, but it didn't swap the correct items. Your tests would probably still pass!
I would expect this test to check for more detail... Ideally, this test would also check that the contents of inventorys are what you expect them to be.
| assert tai.inventory == [item_b, item_a, item_f] | ||
| assert jesse.inventory == [item_e, item_d, item_c] |
There was a problem hiding this comment.
To drive a point home...
These lines are correct, they're checking if tai.inventory is the correct value. In this situation, it's possible that we simply want to check that the right items are in the inventory, without relying on correct ordering.
Consider this alternative:
assert item_b in tai.inventory
assert item_a in tai.inventory
assert item_f in tai.inventory
# ... etc2 participants
No description provided.