Scissors Swap Meet - Araceli #71
Conversation
There was a problem hiding this comment.
A lot of what's here looks good. You successfully wrote classes with instance methods that appropriately interact with instance variables, which is great!
That said there are a few learning goals that aren't met as the code is written:
- There is no usage of inheritance (and therefore no chance to override methods)
- There are several missed opportunities to DRY up the code by calling instance methods from other methods
- 3 of the methods don't do what the README and the tests direct them to
If you can, I would encourage you to see if you can update the code to address each of these things to solidly fulfill the learning goals for this project.
| import pytest | ||
| from swap_meet.vendor import Vendor | ||
| from swap_meet.item import Item | ||
|
|
||
|
|
||
| item_a = Item(category="clothing") | ||
| item_b = Item(category="electronics") | ||
| item_c = Item(category="clothing") | ||
| vendor = Vendor( | ||
| inventory=[item_a, item_b, item_c] | ||
| ) | ||
|
|
||
| items = vendor.get_by_category("clothing") |
There was a problem hiding this comment.
Love that you did some of your own experimenting with a separate main.py file!
swap_meet/clothing.py
Outdated
| if self.condition == 1: | ||
| return "Geez, maybe put that back." | ||
| elif self.condition == 2: | ||
| return "Well, could be worse." | ||
| elif self.condition == 3: | ||
| return "What are the savings from retail value?" | ||
| elif self.condition == 4: | ||
| return "Almost in perfect condition" | ||
| elif self.condition == 5: | ||
| return "I am delighted from the sun and back" |
swap_meet/decor.py
Outdated
| if self.condition == 1: | ||
| return "Geez, maybe put that back." | ||
| elif self.condition == 2: | ||
| return "Well, could be worse." | ||
| elif self.condition == 3: | ||
| return "What are the savings from retail value?" | ||
| elif self.condition == 4: | ||
| return "Almost in perfect condition" | ||
| elif self.condition == 5: | ||
| return "I am delighted from the sun and back" |
There was a problem hiding this comment.
If the Clothing, Decor, and Electronics classes were written to inherit from Item, this method could go in the Item class instead and it wouldn't need to be repeated in 3 different classes. I would encourage you to take time to see if you can rewrite these classes to use inheritance.
swap_meet/decor.py
Outdated
| @@ -0,0 +1,22 @@ | |||
| class Decor: | |||
There was a problem hiding this comment.
This is the first of many changes that'd need to be made to this class definition to properly inherit from the Item class.
| class Decor: | |
| class Decor(Item): |
swap_meet/vendor.py
Outdated
| self.my_item = my_item | ||
| self.their_item = their_item | ||
|
|
||
| if my_item == True and their_item == True: |
There was a problem hiding this comment.
This will never evaluate to true so the method will never actually appropriately swap the items.
swap_meet/vendor.py
Outdated
|
|
||
| self.inventory[0] = their_first_item | ||
| other.inventory[0] = my_first_item | ||
| return bool(self.inventory) |
There was a problem hiding this comment.
I'm curious to know why this is written this way. This will return true as long as the list isn't empty, and there's in if check above to already ensure the list isn't empty, so this should always return True anyway.
| return bool(self.inventory) | |
| return True |
swap_meet/vendor.py
Outdated
| for item in self.inventory: | ||
| if item.category == category: | ||
| best_in_category.append(item) |
There was a problem hiding this comment.
With the conditional on line 71, there's actually no need for this step. Right now the same check is happening twice.
| for item in self.inventory: | |
| if item.category == category: | |
| best_in_category.append(item) |
swap_meet/vendor.py
Outdated
| if item.category == category: | ||
| best_in_category.append(item) | ||
|
|
||
| for item in best_in_category: |
There was a problem hiding this comment.
| for item in best_in_category: | |
| for item in self.inventory: |
swap_meet/vendor.py
Outdated
| for condition in best_in_category: | ||
| if item.condition == 5.0: | ||
| return item | ||
| elif item.condition == 4.0: | ||
| return item | ||
| elif item.condition == 3.0: | ||
| return item | ||
| elif item.condition == 2.0: | ||
| return item | ||
| elif item.condition == 1.0: | ||
| return itme |
There was a problem hiding this comment.
This will always return the first item in the list, no matter what.
I'd encourage you to run the tests with the debugger and seek to understand how this is working and why it isn't doing what you intended.
swap_meet/vendor.py
Outdated
| if my_priority not in other.inventory or their_priority not in self.inventory: | ||
| return False | ||
| else: | ||
| return True |
There was a problem hiding this comment.
There's no swapping actually happening here. If swap_items were rewritten to work properly, we could make use of that method by calling it from here.
No description provided.