-
Notifications
You must be signed in to change notification settings - Fork 26
Sphinx Class_Astry & Rong #7
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
3dd47c0
1325e86
c6954f9
7ff7d4a
9d1cf6d
24ca3c2
b424232
6d8a5cd
e5098c1
007f858
5549cb2
1bdb0d4
eed1006
51276d6
78449fb
b0317b8
cac7fc2
7f6391b
01be4fb
f6aca3d
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,10 @@ | ||
| class Clothing: | ||
| pass | ||
| from swap_meet.item import Item | ||
|
|
||
| class Clothing(Item): | ||
| def __init__(self, id=None, fabric="Unknown", condition=0): | ||
| super().__init__(id=id, condition=condition) | ||
|
Comment on lines
+4
to
+5
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 work on here! On line 4 you correctly set the default value for On line 5 you properly call the initializer of the parent, and pass along the values the child wants to use. |
||
| self.fabric = fabric | ||
|
|
||
| def __str__(self): | ||
| return f"An object of type Clothing with id {self.id}. It is made from {self.fabric} fabric." | ||
|
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 method looks similar to the method you wrote in the In that class What if the Currently, it's not possible because After def __str__(self):
type_str = super().__str__()
fabric_str = f"It is made from {self.fabric} fabric."
return " ".join([type_str, fabric_str]) |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,11 @@ | ||
| 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=id, condition=condition) | ||
| self.width = width | ||
| self.length = length | ||
|
|
||
| def __str__(self): | ||
|
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. See comment from |
||
| return f"An object of type Decor with id {self.id}. It takes up a {self.width} by {self.length} sized space." | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,9 @@ | ||
| class Electronics: | ||
| pass | ||
| from swap_meet.item import Item | ||
|
|
||
| class Electronics(Item): | ||
| def __init__(self, id=None, type="Unknown", condition=0): | ||
| super().__init__(id=id, condition=condition) | ||
| self.type = type | ||
|
|
||
| def __str__(self): | ||
|
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. Prefer to override parent class's implementation of |
||
| return f"An object of type Electronics with id {self.id}. This is a {self.type} device." | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,2 +1,35 @@ | ||||||
| import uuid | ||||||
|
|
||||||
| class Item: | ||||||
| pass | ||||||
| def __init__(self,id=None, condition=0, age=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.
Suggested change
Nit: You're missing a whitespace after the first comma. Be mindful to use/exclude whitespaces where necessary. It is easy to overlook, but having inconsistent use of whitespaces leads to a messy codebase. Here's the section on whitespaces in the PEP8 styleguide. 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 work tackling the optional enhancements relating to an item's age. You set a default param Is there a way to pass in an age argument to subclass constructors? We need a way to set the age of an subclass when we create an instance of it. As item_clothing1 = Clothing(condition=1.0, id=123, fabric="Geometric Pattern", age=3)because I'd get an error that I supplied too many arguments to the constructor. How would you update the subclasses so that |
||||||
| self.id = id if id is not None else uuid.uuid4().int | ||||||
| self.condition = condition | ||||||
| self.age = age | ||||||
|
|
||||||
| def get_category(self): | ||||||
| return self.__class__.__name__ | ||||||
|
Comment on lines
+9
to
+10
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 job writing this method in the super class and not repeating the logic in the sub classes 👍 |
||||||
| # The method get_category() defined in the parent class already works correctly for child classes | ||||||
| # because self.__class__.__name__ reflects the class of the instance, not the class where the method is defined. | ||||||
| # Python dynamically resolves self.__class__.__name__ at runtime, | ||||||
| # so it always returns the correct class name for the current instance, regardless of where the method is defined. | ||||||
|
|
||||||
|
|
||||||
| def __str__(self): | ||||||
| return f"An object of type Item with id {self.id}." | ||||||
|
Comment on lines
+17
to
+18
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 mentioned in the children classes of How can you use string interpolation here so that the word "Item" is not hardcoded? How can you make use of the instance method |
||||||
|
|
||||||
| def condition_description(self): | ||||||
| if self.condition == 0: | ||||||
| return "Broken" | ||||||
| elif self.condition == 1: | ||||||
| return "Poor" | ||||||
| elif self.condition == 2: | ||||||
| return "Fair" | ||||||
| elif self.condition == 3: | ||||||
| return "Good" | ||||||
| elif self.condition == 4: | ||||||
| return "Like New" | ||||||
| elif self.condition == 5: | ||||||
| return "New, In Box" | ||||||
|
Comment on lines
+21
to
+32
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 implementation works! It is a little repetitive, and could be considered brittle (prone to breaking) since the data relationships are encoded here (mapping a number to a string). What data structures do you know of that could capture such an associative relationship like this? How about a dictionary with its key/value pairs? ITEM_CONDITIONS = {
0: "Broken",
1: "Poor",
2: "Fair", ...
}
return ITEMS_CONDITIONS.get(self.condition)Having a clear representation of the data can make code easier to read and maintain than complex conditionals. |
||||||
|
|
||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,117 @@ | ||
| class Vendor: | ||
| pass | ||
| def __init__(self, inventory=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. Nit: remove trailing comma after |
||
| inventory = [] if inventory is None else inventory | ||
| self.inventory = inventory | ||
|
Comment on lines
+3
to
+4
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 work using a ternary to handle the default inventory case. You can combine lines 3-4 together: self.inventory = [] if inventory is None else inventory |
||
|
|
||
| def add(self, item): | ||
| self.inventory.append(item) | ||
| return item | ||
|
|
||
| def remove(self, item): | ||
| if item not in self.inventory: | ||
| return False | ||
| else: | ||
| self.inventory.remove(item) | ||
| return item | ||
|
Comment on lines
+10
to
+15
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. Since you've reversed the logic to put the error checking first as a "guard clause" this allows us to unindent the main logic and remove if item not in self.inventory:
return False
self.inventory.remove(item)
return item |
||
|
|
||
| def get_by_id(self, id): | ||
| for item in self.inventory: | ||
| if item.id == id: | ||
| return item | ||
|
|
||
| return None | ||
|
|
||
| def swap_items(self, other_vendor, my_item, their_item): | ||
| if my_item in self.inventory and their_item in other_vendor.inventory: | ||
| self.inventory.remove(my_item) | ||
| self.inventory.append(their_item) | ||
| other_vendor.inventory.append(my_item) | ||
| other_vendor.inventory.remove(their_item) | ||
|
Comment on lines
+26
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. Removing and appending elements to a list works! If you haven't seen it already, Python also has syntax for assigning multiple values in one line that we can leverage to swap elements in a list (read more here) For example: a, b = 100, 200
print(a)
# 100
print(b)
# 200We can use that syntax to swap items like so: my_item_index = self.inventory.index(my_item)
their_item_index = other_vendor.inventory.index(their_item)
other_vendor.inventory[their_item_index], self.inventory[my_item_index] = my_item, their_item |
||
| return True | ||
| else: | ||
| return False | ||
|
Comment on lines
+31
to
+32
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. Since the logic that swaps items and returns Prefer to have fewer levels of nested logic where possible. Alternatively, the approach I prefer is to use a guard clause to first check if the items are not in their respective lists and return def swap_items(self, other_vendor, my_item, their_item):
if my_item not in self.inventory and their_item not in other_vendor.inventory:
return False
self.inventory.remove(my_item)
self.inventory.append(their_item)
other_vendor.inventory.append(my_item)
other_vendor.inventory.remove(their_item)
return True
|
||
|
|
||
| def swap_first_item(self, other_vendor): | ||
| # original way: | ||
| # if not self.inventory or not other_vendor.inventory: | ||
| # return False | ||
| # else: | ||
| # other_vendor.inventory.append(self.inventory[0]) | ||
| # self.inventory.remove(self.inventory[0]) | ||
| # self.inventory.append(other_vendor.inventory[0]) | ||
| # other_vendor.inventory.remove(other_vendor.inventory[0]) | ||
| # return True | ||
|
|
||
| if not self.inventory or not other_vendor.inventory: | ||
| return False | ||
|
Comment on lines
+45
to
+46
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 check that the inventories aren't empty, which would cause the [0] accesses to fail. |
||
| else: | ||
| my_first_item = self.inventory[0] | ||
| their_first_item = other_vendor.inventory[0] | ||
| self.swap_items(other_vendor, my_first_item, their_first_item) | ||
| return True | ||
|
Comment on lines
+47
to
+51
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. Since you use the guard clause pattern on lines 45-46, we should remove Good work using the |
||
|
|
||
| def get_by_category(self, category): | ||
| category_list = [] | ||
|
|
||
| for item in self.inventory: | ||
| if category == item.get_category(): | ||
| category_list.append(item) | ||
|
|
||
| return category_list | ||
|
Comment on lines
+54
to
+60
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. Instead of creating an empty list and appending to it in a for-loop, how could you use list comprehension instead? |
||
|
|
||
| def get_best_by_category(self, category): | ||
| best_item = self.inventory[0] | ||
|
|
||
| for item in self.inventory: | ||
| if category == item.get_category() and item.condition > best_item.condition: | ||
|
Comment on lines
+65
to
+66
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. How could you simplify this logic so that you loop over a list of items belonging to the |
||
| best_item = item | ||
|
|
||
| if best_item.get_category() == category: | ||
|
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. If you start out with only a list of items belonging to the specified category, then we can get rid of this additional check on line 69 too and further reduce the complexity of this method. Ultimately, def get_best_by_category(self, category):
category_items = self.get_by_category(category)
if not category_items:
return None
highest_item = None
highest_rating = 0
for item in item_list:
if item.condition > highest_rating:
highest_item = item
highest_rating = item.condition
return highest_item |
||
| return best_item | ||
| else: | ||
| return None | ||
|
|
||
|
|
||
| def swap_best_by_category(self, other_vendor, my_priority, their_priority): | ||
| if not other_vendor.inventory or not self.inventory: | ||
| return False | ||
|
|
||
| my_item_to_swap = self.get_best_by_category(their_priority) | ||
| their_item_to_swap = other_vendor.get_best_by_category(my_priority) | ||
|
|
||
| if my_item_to_swap and their_item_to_swap: | ||
| self.swap_items(other_vendor, my_item_to_swap, their_item_to_swap) | ||
| return True | ||
| elif not my_item_to_swap or not their_item_to_swap: | ||
| return False | ||
|
Comment on lines
+82
to
+86
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. Prefer to have fewest levels of indentation and remove Lines 82-86 can be refactored to look like this: if not my_item_to_swap or not their_item_to_swap:
return False
self.swap_items(other_vendor, my_item_to_swap, their_item_to_swap)
return True |
||
|
|
||
| # Optional | ||
| # get the newest item and then swap the item | ||
| # situation haven't consider: | ||
| # 1. if there're more thean one item with same age? | ||
| # 2. if my inventory or their inventory is empty | ||
|
|
||
| def get_newest_item(self): | ||
| newest_item = self.inventory[0] | ||
|
|
||
| for item in self.inventory: | ||
| if item.age < newest_item.age: | ||
| newest_item = item | ||
|
|
||
| return newest_item | ||
|
|
||
| def swap_by_newest(self, other_vendor): | ||
| my_newest_item = self.get_newest_item() | ||
| their_newest_item = other_vendor.get_newest_item() | ||
|
|
||
| return self.swap_items(other_vendor, my_newest_item, their_newest_item) | ||
|
Comment on lines
+94
to
+107
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. 👍 LGTM! |
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
Comment on lines
+108
to
+116
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. Only need one blank line after the end of your code in a file. You can remove the excess blank lines to keep your project neat. |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| import pytest | ||
| from swap_meet.item import Item | ||
| from swap_meet.vendor import Vendor | ||
|
|
||
| def test_swap_by_newest(): | ||
| item_a = Item(age=2) | ||
| item_b = Item(age=4) | ||
| item_c = Item(age=3) | ||
| fatimah = Vendor( | ||
| inventory=[item_a, item_b, item_c] | ||
| ) | ||
|
|
||
| item_d = Item(age=1) | ||
| item_e = Item(age=4) | ||
| jolie = Vendor( | ||
| inventory=[item_d, item_e] | ||
| ) | ||
|
|
||
| result = fatimah.swap_by_newest(jolie) | ||
|
|
||
| assert len(fatimah.inventory) == 3 | ||
| assert item_b in fatimah.inventory | ||
| assert item_a not in fatimah.inventory | ||
| assert item_c in fatimah.inventory | ||
| assert item_d in fatimah.inventory | ||
| assert len(jolie.inventory) == 2 | ||
| assert item_d not in jolie.inventory | ||
| assert item_e in jolie.inventory | ||
| assert item_a in jolie.inventory | ||
| assert result | ||
|
Comment on lines
+21
to
+30
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. LGTM! Gad you got some additional practice with this optional enhancement! |
||
| 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_is_false(): | ||
| item = "item to remove" | ||
| vendor = Vendor( | ||
|
|
@@ -49,7 +49,4 @@ def test_removing_not_found_is_false(): | |
|
|
||
| result = vendor.remove(item) | ||
|
|
||
| raise Exception("Complete this test according to comments below.") | ||
| # ********************************************************************* | ||
| # ****** Complete Assert Portion of this test ********** | ||
| # ********************************************************************* | ||
| assert result == False | ||
|
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. Since the general intention of this method is to modify the inventory, we want to check this condition as well to make sure that the items that started in the inventory are still there. Adding these assertions would make the test more robust. assert len(vendor.inventory) == 3
assert "a" in vendor.inventory
assert "b" in vendor.inventory
assert "c" in vendor.inventory |
||
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.
Nice work importing
Itemsince we're using by name as the parent class ofClothing👍