-
Notifications
You must be signed in to change notification settings - Fork 21
"Octothorpes - Elaine(SiOk) Sohn(she/her)" #20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
6179dac
6bc4828
86f3824
37c5753
cfebc75
af03dc3
69d60cc
1527722
1dfbd5f
c99e043
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,9 @@ | ||
| class Clothing: | ||
| pass | ||
| from swap_meet.item import Item | ||
|
|
||
| class Clothing(Item): | ||
| def __init__(self, id=None, fabric="Unknown", condition=0): | ||
| super().__init__(id, condition) | ||
| self.fabric = fabric | ||
|
|
||
| def __str__(self): | ||
| return super().__str__() + f'. It is made from {self.fabric} fabric.' | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,10 @@ | ||
| class Decor: | ||
| pass | ||
| from swap_meet.item import Item | ||
|
|
||
| class Decor(Item): | ||
| def __init__(self, id=None, width=0, length=0, condition=0): | ||
| super().__init__(id, condition) | ||
| self.width = width | ||
| self.length = length | ||
|
|
||
| def __str__(self): | ||
| return super().__str__() + f'. It takes up a {self.width} by {self.length} sized space.' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,13 @@ | ||
| class Electronics: | ||
| pass | ||
| from swap_meet.item import Item | ||
|
|
||
| class Electronics(Item): | ||
| def __init__(self, id=None, type="Unknown", condition=0): | ||
| super().__init__(id, condition) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice use of inheritance across the child classes 👍 |
||
| self.type = type | ||
|
|
||
| def get_category(self): | ||
| return f'{self.__class__.__name__}' | ||
|
Comment on lines
+8
to
+9
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like |
||
|
|
||
| def __str__(self): | ||
| return super().__str__() + f'. This is a {self.type} device.' | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,30 @@ | ||
| class Item: | ||
| pass | ||
| import uuid | ||
|
|
||
| class Item: | ||
| def __init__(self, id=None, condition=0): | ||
| if not id: | ||
| id = uuid.uuid4().int | ||
| self.id = id | ||
|
Comment on lines
+5
to
+7
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice solution, another option here could use conditional assignment: # Assigns self.id to the parameter id if id is truthy,
# otherwise assigns self.id the return value of uuid.uuid4().int
self.id = id if id else uuid.uuid4().int |
||
| self.condition = condition | ||
|
|
||
| def get_category(self): | ||
| return self.__class__.__name__ | ||
|
|
||
| def __str__(self): | ||
| return f"An object of type {self.get_category()} with id {self.id}" | ||
|
|
||
| def condition_description(self): | ||
| if self.condition == 0: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice handling for condition description! It looks like the code assumes that |
||
| return f'Heavily used' | ||
| elif self.condition == 1: | ||
| return f'Acceptable' | ||
| elif self.condition == 2: | ||
| return f'Good' | ||
| elif self.condition == 3: | ||
| return f'Very good' | ||
| elif self.condition == 4: | ||
| return f'Like new' | ||
| else: | ||
| return f"Brand New" | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,103 @@ | ||
| class Vendor: | ||
| pass | ||
| #default value should not be mutable!!! | ||
| def __init__(self, inventory=None): | ||
| if not inventory: | ||
| self.inventory = [] | ||
| else: | ||
| self.inventory = inventory | ||
|
|
||
| def add(self, item): | ||
| self.inventory.append(item) | ||
| return item | ||
|
|
||
| def remove(self, item): | ||
| if item not in self.inventory: | ||
| return None | ||
| self.inventory.remove(item) | ||
| return item | ||
|
|
||
| def get_by_id(self, id): #if id is None? ==> wave02 | ||
| for item in self.inventory: | ||
| if item.id == id: | ||
| return item | ||
| return None | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like that you're explicitly returning |
||
|
|
||
| def swap_items(self, other_vendor, my_item, their_item): | ||
| if my_item not in self.inventory or their_item not in other_vendor.inventory: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With indentation this line is a little long, I would consider breaking it up at the if (my_item not in self.inventory
or their_item not in other_vendor.inventory):
return FalseThe PEP8 style guide doesn't give a strict recommendation on how to break an if-statement across multiple lines, but they give a few suggestions in the second half of the section on indentation: https://peps.python.org/pep-0008/#indentation |
||
| return False | ||
| other_vendor.add(my_item) | ||
| self.remove(my_item) | ||
|
Comment on lines
+28
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice use of the Vendor methods 🙂 |
||
| self.add(their_item) | ||
| other_vendor.remove(their_item) | ||
| return True | ||
|
|
||
| #TODO reuse swap_itme() | ||
| def swap_first_item(self, other_vendor): | ||
| if not other_vendor.inventory or not self.inventory: | ||
| return False | ||
| first_my_item = self.remove(self.inventory[0]) | ||
| other_vendor.add(first_my_item) | ||
| first_their_item = other_vendor.remove(other_vendor.inventory[0]) | ||
| self.add(first_their_item) | ||
|
Comment on lines
+38
to
+41
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see you have a note above about reusing |
||
| return True | ||
|
|
||
| def get_by_category(self, category): | ||
| items = [] | ||
| for item in self.inventory: | ||
| if item.get_category() == category: | ||
| items.append(item) | ||
| return items | ||
|
Comment on lines
+45
to
+49
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Notice this pattern of: result_list = []
for element in source_list:
if some_condition(element):
result_list.append(element)can be rewritten using a list comprehension as: result_list = [element for element in source_list if some_condition(element)]Which here would look like: items = [item for item in self.inventory if item.get_category() == category]At first, this may seem more difficult to read, but comprehensions are a very common python style, so I encourage y’all to practice working with them! |
||
|
|
||
| def get_best_by_category(self, category=""): | ||
| items = self.get_by_category(category) | ||
| best_condition = 0.0 | ||
| best_item = None | ||
| for item in items: | ||
| if item.condition > best_condition: | ||
| best_item = item | ||
| best_condition = item.condition | ||
| return best_item | ||
|
Comment on lines
+55
to
+59
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice loop! The time complexity would be the same, but another way we could write this is using the return max(items, key=lambda item: item.condition) |
||
|
|
||
| def swap_best_by_category(self, other_vendor, my_priority, their_priority): | ||
| my_item = self.get_best_by_category(their_priority) | ||
| their_item = other_vendor.get_best_by_category(my_priority) | ||
|
|
||
| return self.swap_items(other_vendor, my_item, their_item) | ||
|
|
||
| #wave 07 | ||
| def display_inventory(self, category=""): | ||
| if not self.inventory: | ||
| print("No inventory to display.") | ||
| elif not category: | ||
| for i, item in enumerate(self.inventory): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great use for enumerate! If we don't want to manually add the |
||
| print(f"{i+1}. {str(item)}") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because we have implemented the print(f"{i+1}. {item}") |
||
| else: | ||
| display_items = self.get_by_category(category) | ||
| if not display_items: | ||
| print("No inventory to display.") | ||
| else: | ||
| for i, item in enumerate(display_items): | ||
| print(f"{i+1}. {str(item)}") | ||
|
Comment on lines
+69
to
+80
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This if-tree is pretty nested and we repeat our looping and no inventory code. If we first resolve what list of items we want to iterate over, then error handle if it is empty, we can remove some of the if-elses: # If `category` is not empty or None, get a list of items with that category,
# otherwise set `display_items` to `self.inventory`
display_items = self.get_by_category(category) if category else self.inventory
# The line above would be equivalent to
# if category:
# display_items = self.get_by_category(category)
# else:
# display_items = self.inventory
# We can now remove code that was repeated when the list of items was empty
if not display_items:
print("No inventory to display.")
return
# If we've gotten here, there must be an inventory to print
for index, item in enumerate(display_items, start=1):
print(f"{index}. {item}") |
||
|
|
||
| def swap_by_id(self, other_vendor, my_item_id,their_item_id): | ||
| my_item = self.get_by_id(my_item_id) | ||
| their_item = other_vendor.get_by_id(their_item_id) | ||
|
|
||
| return self.swap_items(other_vendor, my_item, their_item) | ||
|
|
||
| def choose_and_swap_items(self, other_vendor, category=""): | ||
|
|
||
| self.display_inventory(category) | ||
| my_item_id = int(input("Enter an item id from your inventory: ")) | ||
|
|
||
| other_vendor.display_inventory(category) | ||
| their_item_id = int(input("Enter an item id from your friend's inventory: ")) | ||
|
|
||
| return self.swap_by_id(other_vendor, my_item_id, their_item_id) | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,12 +2,12 @@ | |
| import pytest | ||
| from swap_meet.vendor import Vendor | ||
|
|
||
| @pytest.mark.skip | ||
| # @pytest.mark.skip | ||
| def test_vendor_has_inventory(): | ||
| vendor = Vendor() | ||
| assert len(vendor.inventory) == 0 | ||
|
|
||
| @pytest.mark.skip | ||
| # @pytest.mark.skip | ||
| def test_vendor_takes_optional_inventory(): | ||
| inventory = ["a", "b", "c"] | ||
| vendor = Vendor(inventory=inventory) | ||
|
|
@@ -16,7 +16,7 @@ def test_vendor_takes_optional_inventory(): | |
| assert "b" in vendor.inventory | ||
| assert "c" in vendor.inventory | ||
|
|
||
| @pytest.mark.skip | ||
| # @pytest.mark.skip | ||
| def test_adding_to_inventory(): | ||
| vendor = Vendor() | ||
| item = "new item" | ||
|
|
@@ -27,7 +27,7 @@ def test_adding_to_inventory(): | |
| assert item in vendor.inventory | ||
| assert result == item | ||
|
|
||
| @pytest.mark.skip | ||
| # @pytest.mark.skip | ||
| def test_removing_from_inventory_returns_item(): | ||
| item = "item to remove" | ||
| vendor = Vendor( | ||
|
|
@@ -40,7 +40,7 @@ def test_removing_from_inventory_returns_item(): | |
| assert item not in vendor.inventory | ||
| assert result == item | ||
|
|
||
| @pytest.mark.skip | ||
| # @pytest.mark.skip | ||
| def test_removing_not_found_returns_none(): | ||
| item = "item to remove" | ||
| vendor = Vendor( | ||
|
|
@@ -49,7 +49,9 @@ def test_removing_not_found_returns_none(): | |
|
|
||
| result = vendor.remove(item) | ||
|
|
||
| raise Exception("Complete this test according to comments below.") | ||
| assert result is None | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice assert, we definitely want to confirm our return value is |
||
|
|
||
| # raise Exception("Complete this test according to comments below.") | ||
| # ********************************************************************* | ||
| # ****** Complete Assert Portion of this test ********** | ||
| # ********************************************************************* | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ | |
| from swap_meet.vendor import Vendor | ||
| from swap_meet.item import Item | ||
|
|
||
| @pytest.mark.skip | ||
| # @pytest.mark.skip | ||
| def test_item_overrides_to_string(): | ||
| test_id = 12345 | ||
| item = Item(id=test_id) | ||
|
|
@@ -12,7 +12,7 @@ def test_item_overrides_to_string(): | |
| expected_result = f"An object of type Item with id {test_id}" | ||
| assert item_as_string == expected_result | ||
|
|
||
| @pytest.mark.skip | ||
| # @pytest.mark.skip | ||
| def test_swap_items_returns_true(): | ||
| item_a = Item() | ||
| item_b = Item() | ||
|
|
@@ -40,7 +40,7 @@ def test_swap_items_returns_true(): | |
| assert item_b in jolie.inventory | ||
| assert result | ||
|
|
||
| @pytest.mark.skip | ||
| # @pytest.mark.skip | ||
| def test_swap_items_when_my_item_is_missing_returns_false(): | ||
| item_a = Item() | ||
| item_b = Item() | ||
|
|
@@ -67,7 +67,7 @@ def test_swap_items_when_my_item_is_missing_returns_false(): | |
| assert item_e in jolie.inventory | ||
| assert not result | ||
|
|
||
| @pytest.mark.skip | ||
| # @pytest.mark.skip | ||
| def test_swap_items_when_their_item_is_missing_returns_false(): | ||
| item_a = Item() | ||
| item_b = Item() | ||
|
|
@@ -94,7 +94,7 @@ def test_swap_items_when_their_item_is_missing_returns_false(): | |
| assert item_e in jolie.inventory | ||
| assert not result | ||
|
|
||
| @pytest.mark.skip | ||
| # @pytest.mark.skip | ||
| def test_swap_items_from_my_empty_returns_false(): | ||
| fatimah = Vendor( | ||
| inventory=[] | ||
|
|
@@ -114,7 +114,7 @@ def test_swap_items_from_my_empty_returns_false(): | |
| assert len(jolie.inventory) == 2 | ||
| assert not result | ||
|
|
||
| @pytest.mark.skip | ||
| # @pytest.mark.skip | ||
| def test_swap_items_from_their_empty_returns_false(): | ||
| item_a = Item() | ||
| item_b = Item() | ||
|
|
@@ -131,7 +131,8 @@ def test_swap_items_from_their_empty_returns_false(): | |
|
|
||
| result = fatimah.swap_items(jolie, item_b, nobodys_item) | ||
|
|
||
| raise Exception("Complete this test according to comments below.") | ||
| assert not result | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The feedback in wave 1 applies here as well, what else would be helpful to assert to ensure there were no unintended side effects? |
||
| # raise Exception("Complete this test according to comments below.") | ||
| # ********************************************************************* | ||
| # ****** Complete Assert Portion of this test ********** | ||
| # ********************************************************************* | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are using an f-string already, another option would be to call the superclass function from a placeholder: