-
Notifications
You must be signed in to change notification settings - Fork 113
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
Sharks/C17/Huma Hameed #108
base: master
Are you sure you want to change the base?
Changes from all commits
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,12 @@ | ||
class Clothing: | ||
pass | ||
from swap_meet.item import Item | ||
|
||
class Clothing(Item): | ||
def __init__(self, category = "", condition = 0): | ||
super().__init__(category = "Clothing", condition = condition) | ||
|
||
def __str__(self): | ||
string_item = "The finest clothing you could wear." | ||
return string_item | ||
|
||
def condition_description(self): | ||
return super().condition_description() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,12 @@ | ||
class Decor: | ||
pass | ||
from swap_meet.item import Item | ||
|
||
class Decor(Item): | ||
def __init__(self, category = "", condition = 0): | ||
super().__init__(category = "Decor", condition = condition) | ||
|
||
def __str__(self): | ||
string_item = "Something to decorate your space." | ||
return string_item | ||
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. You can also directly return the string literal like this: def __str__(self):
return "Something to decorate your space." |
||
|
||
def condition_description(self): | ||
return super().condition_description() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,12 @@ | ||
class Electronics: | ||
pass | ||
from swap_meet.item import Item | ||
|
||
class Electronics(Item): | ||
def __init__(self, category = "", condition = 0): | ||
super().__init__(category = "Electronics", condition = condition) | ||
|
||
def __str__(self): | ||
string_item = "A gadget full of buttons and secrets." | ||
return string_item | ||
|
||
def condition_description(self): | ||
return super().condition_description() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,30 @@ | ||
|
||
class 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. Item class looks good 👍 |
||
pass | ||
|
||
|
||
def __init__(self, category = "", condition = 0.0): | ||
""" | ||
def __init__ creates attribute of empty string category and condition with default value | ||
""" | ||
self.category = category | ||
self.condition = condition | ||
|
||
def __str__(self): | ||
string_item = "Hello World!" | ||
return string_item | ||
|
||
def condition_description(self): | ||
""" | ||
def condition_description() creates key/values of level of \ | ||
condition which will be used in other files | ||
""" | ||
CONDITION_DICT = {0: "Brand-spanking new!", | ||
1: "New-but really not", | ||
2: "Just okay", | ||
3: "Not so okay", | ||
4: "Don't know what to tell ya", | ||
5: "Wouldn't touch with a 10ft pole"} | ||
condition_value = CONDITION_DICT[self.condition] | ||
return condition_value | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,104 @@ | ||
from swap_meet.item import Item | ||
|
||
class Vendor: | ||
pass | ||
|
||
|
||
def __init__(self, inventory = None): | ||
""" | ||
def __init__ creates attribute of empty inventory to hold items | ||
""" | ||
if inventory == None: | ||
inventory = [] | ||
self.inventory = inventory | ||
Comment on lines
+10
to
+12
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. Looks good! Another way you might see this written is with a ternary, as:
In this situation, that would look like
|
||
|
||
|
||
def add(self, item): | ||
""" | ||
def add() adds items to inventory and returns the item that was added | ||
""" | ||
self.inventory.append(item) | ||
return item | ||
|
||
|
||
def remove(self, item): | ||
""" | ||
def remove() removes matching item from inventory \ | ||
and returns the item that was removed | ||
""" | ||
if item in self.inventory: | ||
self.inventory.remove(item) | ||
return item | ||
else: | ||
return False | ||
Comment on lines
+28
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. A more explicit way to if item not in self.inventory:
return False
self.inventory.remove(item)
return item Another approach we could take is to try to remove the item directly, and handle the ValueError that occurs if it's not there, and return False to handle it (try/except) |
||
|
||
|
||
def get_by_category(self, input_category): | ||
""" | ||
def get_by_category() takes in a category string \ | ||
and returns a list of items in the inventory with that category | ||
""" | ||
item_list = [] | ||
for item in self.inventory: | ||
if item.category == input_category: | ||
item_list.append(item) | ||
return item_list | ||
Comment on lines
+40
to
+44
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, nice. This method is a great candidate for using list comprehension if you want to refactor it since our conditional statement we check is pretty simple. General syntax for list comp: result_list = [element for element in source_list if some_condition(element)] Which here would look like: item_list = [item for item in self.inventory if item.category == category] You can also forgo saving the list to the variable item_list and do something like: def get_by_category(self, category):
return [item for item in self.inventory if item.category == category] |
||
|
||
|
||
def swap_items(self, other_vendor, my_item, their_item): | ||
""" | ||
def swap_items() returns false if self doesn't contain my_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. you don't need explicit line breaks (backslash) for docstrings |
||
or other_vendor doesn't contain their_item\ | ||
It swaps my_item from self with their_item from other_vendor \ | ||
and returns trues | ||
""" | ||
if my_item not in self.inventory or their_item not in other_vendor.inventory: | ||
return False | ||
Comment on lines
+54
to
+55
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 guard clause |
||
self.add(their_item) | ||
self.remove(my_item) | ||
other_vendor.add(my_item) | ||
other_vendor.remove(their_item) | ||
return True | ||
|
||
|
||
def swap_first_item(self, other_vendor): | ||
""" | ||
def swap_first_item() returns false if self or other_vendor have an empty list\ | ||
It swaps first item from self and other_vendor and swaps those items\ | ||
and returns true | ||
""" | ||
if len(self.inventory) ==0 or len(other_vendor.inventory) ==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. For this guard clause here, it would be more idiomatic to write |
||
return False | ||
my_first_item = self.inventory[0] | ||
friend_first_item = other_vendor.inventory[0] | ||
return self.swap_items(other_vendor, my_first_item, friend_first_item) | ||
|
||
def get_best_by_category(self, category): | ||
""" | ||
def get_best_by_category() returns false if self or other_vendor have an empty list\ | ||
It swaps first item from self and other_vendor and swaps those items\ | ||
and returns true | ||
""" | ||
cat_item_list = self.get_by_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. Nice job reusing a method you already wrote |
||
best_condition = 0 | ||
best_item = None | ||
for item in cat_item_list: | ||
item_condition = item.condition | ||
if best_condition < item_condition: | ||
best_condition = item_condition | ||
best_item = item | ||
return best_item | ||
Comment on lines
+84
to
+89
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. 👍 |
||
|
||
|
||
def swap_best_by_category(self, other, my_priority, their_priority): | ||
""" | ||
def swap_best_by_category() uses get_best_by_category to return a list of items in matching priority category\ | ||
if items don't match priority returns false\ | ||
otherwise swaps items and returns true | ||
""" | ||
my_best = self.get_best_by_category(their_priority) | ||
their_best = other.get_best_by_category(my_priority) | ||
|
||
if my_best == None or their_best == None: | ||
return False | ||
self.swap_items(other, my_best, their_best) | ||
return True | ||
Comment on lines
+101
to
+104
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 of how swap_items is implemented, checking if my_best and their_best are valid on line 101 isn't necessary. See line 54 in vendor.py - you have a check in there that makes sure the args passed into swap_items() are valid. Also swap_items() returns True if swapping happened and False if no swapping happened. Therefore, you can leverage the return value from swap_items() and refactor this method so it looks like this: def swap_best_by_category(self, other, my_priority, their_priority):
my_best = self.get_best_by_category(their_priority)
their_best = other.get_best_by_category(my_priority)
return self.swap_items(other, my_best, their_best) |
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,9 @@ def test_removing_not_found_is_false(): | |
|
||
result = vendor.remove(item) | ||
|
||
raise Exception("Complete this test according to comments below.") | ||
# 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. 👍 |
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 Clothing inherits from Item, an instance of clothing will automatically have access to the parent class methods (like condition_description).
So if you did
pants = Clothing(condition=5)
then you could callpants.condition_description()
even if you had not written lines 11 and 12.What's happening on line 11 and 12 is that you create a condition_description method in Clothing which overrides the method in the Item class. However your method here isn't specialized to the Clothing class since it just returns the value from the calling the parent class's method. If you needed to do something else in addition to line 12 then you would override the parent class's method and write additional logic. However, since you're not changing the functionality, you can remove 11 and 12 entirely.
The same comment applies to Electronics and Decor too.