swap-meet, C17, Sea Turtles, Shelby Faulconer#109
swap-meet, C17, Sea Turtles, Shelby Faulconer#109pickled-bot wants to merge 7 commits intoada-c17:masterfrom
Conversation
…n folder swap_meet. removed skips from the integration test files and unit_test files. added assertions to the test files that were not completed.
…file. I could add this file to the directory but I am not sure it is important to do so.
kelsey-steven-ada
left a comment
kelsey-steven-ada
left a comment
There was a problem hiding this comment.
🎉 You completed Unit 1! 🎉 Great work on this project, I've left some suggestions for refactoring. Take a look, and let me know if anything needs clarification!
|
|
||
| def __init__(self, condition = 0): | ||
| super().__init__(category = "Clothing", condition = condition) | ||
| self.condition = condition |
There was a problem hiding this comment.
We can remove this line like in the Decor & Electronics classes since the condition attribute is set by the super() dunder init function.
| for key, value in CONDITION_DESCRIPTION.items(): | ||
| if self.condition is key: | ||
| return value |
There was a problem hiding this comment.
Since CONDITION_DESCRIPTION is a dictionary, we could simplify this to avoid looping over the values:
| for key, value in CONDITION_DESCRIPTION.items(): | |
| if self.condition is key: | |
| return value | |
| return CONDITION_DESCRIPTION[self.condition] |
Since we don't currently check if the condition passed to an Item is strictly between 0-5, we might want to wrap it in a try/except in case we get a KeyError.
Something else to consider: what would happen if we tried to get the description for an Item with a condition of 3.5? There are tests that set the condition to 3.5, but don't try to read the description. The description test checks for a condition of 1 and a description of 5, but we should still make sure our code covers any other reasonable conditions, including floating point values (since we've seen them used in other tests).
How could we modify this (slightly) to cover the values between the whole numbers?
| @@ -1,2 +1,88 @@ | |||
| from unicodedata import category | |||
There was a problem hiding this comment.
It looks like VS Code might have been a little overzealous with auto-imports. We should check for and remove imports that are unused as part of our code clean up when we think we're about done.
| @@ -1,2 +1,88 @@ | |||
| from unicodedata import category | |||
| from swap_meet.item import Item | |||
There was a problem hiding this comment.
Because of the way python handles typing and looking up attributes & methods, if we do not directly reference a class or its constructor, we do not need to import the class into another file, even if it uses instances of that type. Ansel shared out a slide deck on Slack that talks about how this works that could be helpful if you haven't seen it yet: https://docs.google.com/presentation/d/1PMSNIHgfBViYQ_MlP59qAm5ylT9njZfS9YQ6-xsvKhU/edit?usp=sharing
Here in Vendor, we never create an Item instance or use the Item class itself to do something like call a class method, so we do not need to import item, and should remove this line.
|
|
||
| class Electronics(Item): | ||
|
|
||
| def __init__(self, condition = 0): |
| desired_category = [] | ||
|
|
||
| for item in self.inventory: | ||
| if category in item.category: | ||
| desired_category.append(item) | ||
| # lambda will sort objects by instance's "condtion" attribute | ||
| sort_list = sorted(desired_category, key=lambda item: item.condition) |
There was a problem hiding this comment.
Since we're building up a list of items with a particular category we could reuse the function get_by_category here!
| desired_category = [] | |
| for item in self.inventory: | |
| if category in item.category: | |
| desired_category.append(item) | |
| # lambda will sort objects by instance's "condtion" attribute | |
| sort_list = sorted(desired_category, key=lambda item: item.condition) | |
| desired_category = self.get_by_category(category) | |
| # lambda will sort objects by instance's "condtion" attribute | |
| sort_list = sorted(desired_category, key=lambda item: item.condition) |
| return sort_list[-1] | ||
|
|
||
|
|
||
| def swap_best_by_category(self, other, my_priority, their_priority): |
There was a problem hiding this comment.
Great approach for this function!
| # ****** Complete Assert Portion of this test ********** | ||
| # ********************************************************************* | ||
| # raise Exception("Complete this test according to comments below.") | ||
| assert result == False |
There was a problem hiding this comment.
This assert is great for confirming the return value is correct, but what else could we assert to make sure there were no side affects like changes to the inventory?
| assert tai.inventory == [item_a, item_b, item_f] | ||
| assert jesse.inventory ==[item_d, item_e, item_c] |
There was a problem hiding this comment.
Nice assertions! How could we check the right values are in the right list if we could not guarantee the order of the lists?
| assert len(jesse.inventory) == 3 | ||
| assert len(tai.inventory) == 3 | ||
| assert tai.inventory == [item_a, item_b, item_f] | ||
| assert jesse.inventory ==[item_d, item_e, item_c] |
There was a problem hiding this comment.
Tiny typo that looks like it got copy/pasted to other tests below:
| assert jesse.inventory ==[item_d, item_e, item_c] | |
| assert jesse.inventory == [item_d, item_e, item_c] |
2 participants
No description provided.