Sapphire - Ana S#73
Conversation
… id from inventory
…these modules using parent class Item.
…ap best by category. Added assert statements to tests
…dded doc strings for module vendor and item
tildeee
left a comment
tildeee
left a comment
There was a problem hiding this comment.
Ana! You did great work during this project! This project is getting marked as a "green."
Overall, your solution looks exactly like how I was hoping good OOP code would look. I saw how you practiced using instance methods, working with instances, inheritance, etc etc etc. A few of my comments are about minor things that I've gotten from your code, but nothing major to point out.
I want to do a specific shout out to your optional enhancements and tests. I really appreciate you practicing all of those things, and I hope it also feels like fun, precious practice for you.
In addition, your git hygiene looks great; great commits and commit messages.
Keep it up, Ana! Let me know what questions come up, but otherwise, good work.
| self.inventory = [] | ||
|
|
||
| def add(self, item): | ||
| self.inventory.append(item) |
There was a problem hiding this comment.
Hmm, in which case would item not be in self.inventory? Please correct me if I'm mistaken, but I think we don't need this conditional-- at least, the tests still pass when we take this out.
| if item not in self.inventory: | ||
| return item | ||
|
|
||
| def get_by_id(self, search_id): |
There was a problem hiding this comment.
Great way to express this compound conditional :D
| return None | ||
|
|
||
| def swap_items(self, other_vendor, my_item, their_item): | ||
| if not (my_item in self.inventory and their_item in other_vendor.inventory): | ||
| return False |
There was a problem hiding this comment.
Excellent work :D This is one of the parts with the most work around instance methods and variables.
| self.inventory.append(their_item) | ||
| other_vendor.inventory.append(my_item) | ||
| #Remove their item from their inventory, remove my item from my inventory | ||
| self.inventory.remove(my_item) |
There was a problem hiding this comment.
I like this way of checking if inventory is empty/Falsey
This is a really small piece of knowledge: It's very subtle, but if you change this conditional, you can achieve "short-circuiting". (Sorry if this has already been covered in class and I didn't know.) In compound conditionals with an and, if the first part of the conditional (before the and) is already False, then the program won't even get to the second part after the and, it'll know that the overall conditional is False. Similarly, if it's an or, if the first part is True, then the overall conditional is True. If we were to apply it here, we'd need to refactor your if to:
if not self.inventory or not other_vendor.inventory:
return FalseIn the grand scheme of things, in this project, there's not really an argument to refactor it, I just wanted to use the chance to explain something cool :D . There are random cases where this kind of "trick" is useful, though, but I've seen that like once or twice in my career.
| def get_best_by_category(self, category): | ||
| inventory_by_category = self.get_by_category(category) | ||
| if not inventory_by_category: | ||
| return None |
| vendor_best_item = self.get_best_by_category(their_priority) | ||
| other_vendor_best_item = other_vendor.get_best_by_category(my_priority) |
There was a problem hiding this comment.
[nit] Your other_vendor_best_item variable never gets used... instead, you call it inline as the third argument to swap_items. I like either approach (keeping the local variable and using it or deleting it)
| item_newest = min(inventory_by_category, key=lambda item: item.age) | ||
| return item_newest | ||
|
|
||
| def swap_newest_by_category(self, other_vendor, my_priority, their_priority): |
| #This is a possible higher order function to combine swap by age or condition and category. | ||
| #It is commented out to not change test_wave_06 | ||
| # | ||
| # def swap_modifier_by_category(self, other_vendor, my_priority, their_priority, swap_modifier): | ||
| # if not (self.get_by_category(their_priority) and other_vendor.get_by_category(my_priority)): | ||
| # return False | ||
| # vendor_item = swap_modifier(self, their_priority) | ||
| # other_vendor_item = swap_modifier(other_vendor, my_priority) | ||
| # self.swap_items(other_vendor, vendor_item, other_vendor_item) |
There was a problem hiding this comment.
This is FANTASTIC! I'm so glad you got to practice higher-order functions. I love this, great work, keep this up!!
| pedro = Vendor( | ||
| inventory=[item_a, item_b, item_c, item_d, item_e] | ||
| ) | ||
|
|
There was a problem hiding this comment.
Hmm, this test is called test_newest_by_category but it calls get_best_by_category?
| assert item_b in pedro.inventory | ||
| assert item_c in pedro.inventory | ||
| assert item_d in pedro.inventory | ||
|
|
||
| assert item_a in juan.inventory | ||
| assert item_e in juan.inventory |
There was a problem hiding this comment.
Nicely done! I like how you set up your test!
2 participants
No description provided.