Skip to content
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

sea-turtles, jae clayton #16

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions main.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
from swap_meet.item import Item
from swap_meet.vendor import Vendor
from swap_meet.clothing import Clothing
from swap_meet.decor import Decor
from swap_meet.electronics import Electronics








16 changes: 14 additions & 2 deletions swap_meet/clothing.py
Original file line number Diff line number Diff line change
@@ -1,2 +1,14 @@
class Clothing:
pass
from swap_meet.item import Item

class Clothing(Item):
Comment on lines +1 to +3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

We definitely need to import Item so that it can be the parent class of Clothing.

def __init__(self,condition=0.0):
super().__init__(category= 'Clothing', condition=condition)
Comment on lines +4 to +5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Nice job removing the category from the constructor, since we don't want to encourager the caller to try to change it. Instead, we pass the literal value that we want to set up to the parent's constructor.




def __str__(self):
return "The finest clothing you could wear."


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notice that we don't need to define condition_description. Since we want the same implementation for this child class as for the parent class, by leaving it out of the class definition, we will inherit the method from the parent.



12 changes: 10 additions & 2 deletions swap_meet/decor.py
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, condition=0.0):
super().__init__(category= 'Decor', condition=condition)



def __str__(self):
return "Something to decorate your space."
13 changes: 11 additions & 2 deletions swap_meet/electronics.py
Original file line number Diff line number Diff line change
@@ -1,2 +1,11 @@
class Electronics:
pass
from swap_meet.item import Item

class Electronics(Item):
def __init__(self, condition=0.0):
super().__init__(category = "Electronics", condition=condition)




def __str__(self):
return "A gadget full of buttons and secrets."
50 changes: 49 additions & 1 deletion swap_meet/item.py
Original file line number Diff line number Diff line change
@@ -1,2 +1,50 @@
# from .clothing import Clothing
# from .decor import Decor
# from .electronics import Electronics
Comment on lines +1 to +3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for all of this (and would cause a circular import error if not commented) so best to remove it.


class Item:
pass
def __init__(self, category = '', condition=0.0):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

self.category = category
self.condition = condition


def __str__(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

return "Hello World!"

def condition_description(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be returned if the condition were say -1, or how about 10.

Also, notice how repetitive all the conditions are. If we wanted to add additional conditions, we would need to write more code to handle them. What if we stored the conditions in a collection and could somehow calculate where in the collection the appropriate description for each condition could be found? If we do it carefully, adding additional conditions could be as simple as adding a new string to the collection, and there might be less code overall that would be easier to test (are ALL of these messages currently being tested?).

if self.condition <= 0:
return "disgusting"
if self.condition <= 1:
return "needs a wash"
if self.condition <= 2:
return "passable"
if self.condition <= 3:
return "a bargain"
if self.condition <= 4:
return "gross"
if self.condition <= 5:
return "mint!"


'''
All three classes and the Item class have an instance method
named condition_description, which should describe the condition
in words based on the value, assuming they all range from 0 to 5.
These can be basic descriptions (eg. 'mint', 'heavily used') but
feel free to have fun with these
(e.g. 'You probably want a glove for this one...").
The one requirement is that the condition_description for
all three classes above have the same behavior.
'''
Comment on lines +29 to +38
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably list these as comments rather than a string.













94 changes: 93 additions & 1 deletion swap_meet/vendor.py
Original file line number Diff line number Diff line change
@@ -1,2 +1,94 @@
from swap_meet.item import Item

class Vendor:
pass
def __init__(self, inventory=None):
self.inventory = inventory if inventory is not None else []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice approach to ensure that a missing inventory parameter results in an empty inventory by default.


@staticmethod
def check_if_in_inventory(inventory, item):
if item not in inventory:
return False
return True
Comment on lines +7 to +11
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This definitely works, and correctly applies the @staticmethod decorator, but personally I don't see much use for this function. This can be rewritten as

    @staticmethod
    def check_if_in_inventory(inventory, item):  
        return item in inventory

Which means that anywhere this gets used as

    if Vendor.check_if_in_inventory(some_vendor.inventory, some_item):

It could have been written as

    if some_item in some_vendor.inventory:

Which I find easier to read.


# pass in vendors as tuple, and items as tuple to iterate through them
# just have one static method

@staticmethod
def combo_static_combo(vendor_1, vendor_2, item_1, item_2):
if Vendor.check_if_in_inventory(vendor_1.inventory, item_1) and Vendor.check_if_in_inventory(vendor_2.inventory, item_2):
return True
return False
Comment on lines +16 to +20
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's more of a case to made for this helper than the previous, though again

    if item_1 in vendor_1.inventory and item_2 in vendor_2.inventory:

is being replaced with

    if Vendor.combo_static_combo(vendor_1, vendor_2, item_1, item2): 

To me, the non-helper version is still clearer, since it explicitly shows us which item is being checked against which inventory. For the helper version, I almost feel like a more natural parameter ordering would be

    if Vendor.combo_static_combo(vendor_1, item_1, vendor_2, item2): 

So there's a very real possibility of a caller using the function incorrectly. Also, the name of the function doesn't give a very clear picture of what the function does. Maybe something like are_both_items_in_stock. which would help, but the ordering is still an issue for me.



def add(self, item):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

self.inventory.append(item)
return item


def remove(self, item):
if Vendor.check_if_in_inventory(self.inventory, item):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the discussion above, this could be written as

        if item in self.inventory: 

self.inventory.remove(item)
return item
return False


def get_by_category(self, category = ''):
category_list = []

for item in self.inventory:
if item.category == category:
category_list.append(item)
Comment on lines +36 to +40
Copy link
Contributor

Choose a reason for hiding this comment

The 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 as a list comprehension as

result_list = [element for element in source_list if some_condition(element)]

Which here would look like

        category_list = [item for item in self.inventory if item.category == category]

At first, this may seem more difficult to read, but comprehensions are very common python style, so try getting used to working with them!

return category_list

def swap_items(self, vendor, my_item, their_item):
if Vendor.combo_static_combo(self, vendor, my_item, their_item):

vendor.inventory.append(my_item)
self.inventory.remove(my_item)
self.inventory.append(their_item)
vendor.inventory.remove(their_item)
return True
return False
Comment on lines +44 to +51
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nice thing about using this static method is that it makes it a little easier to invert the condition to turn this into a guard clause scenario.

        if not Vendor.combo_static_combo(self, vendor, my_item, their_item):    
            return False
        
        vendor.inventory.append(my_item)
        self.inventory.remove(my_item)
        self.inventory.append(their_item)
        vendor.inventory.remove(their_item)
        return True 

Alternatively, without the helper, the first check could be written as

        # if it's not the case that both vendors have their needed items
        if not (my_item in self.inventory and their_item in vendor.inventory):

or a little more cleanly as

        # if either vendor doesn't have their needed item
        if my_item not in self.inventory or their_item not in vendor.inventory:

Notice how when moving the not into each partial condition, the and became an or. A similar change happens in the way we describe the situations in English.


def swap_first_item(self, vendor):
if self.inventory == [] or vendor.inventory == []:
return False
Comment on lines +54 to +55
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice guard clause to protect against empty lists.

The more pythonic way to write this would be

        if not self.inventory or not vendor.inventory: 
            return False


first_self = self.inventory[0]
first_vendor = vendor.inventory[0]

item_swap = self.swap_items(vendor, first_self, first_vendor)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice reuse of your swap_items. Notice that due to the remove operations in swap_items, it is a linear operation, regardless of where in the list each item is. We could achieve constant time here either by swapping the items directly, or by doing an index then swap approach in swap_items.

return True


def get_best_by_category(self, category = ''):
if self.inventory == []:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For each of these empty list checks, the more pythonic way to write this would be:

    if not my_list:
        # do whatever

return None


item_list = self.get_by_category(category)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice reuse of your category filtering function.

if len(item_list) == 0:
return None


item_dict = {}
for item in item_list:
item_dict[item] = item.condition

# item_dict = dict.fromkeys{item_list, item.condition }
# item_dict = {item.condition for (item, item.condition in item_list}

best_score = max(item_dict, key=item_dict.get)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be a little careful here. This works and is correct, but requires some understanding of how Python method bindings work. Make sure you do some research in that direction before relying on something like this too heavily.

We could approach this with a more vanilla max() call using the item list (avoid needing to build a dictionary) a lambda to retrieve the condition from each item.

        best_score = max(item_list, key=lambda item: item.condition)

And if we're not comfortable with lambda it could still be written with a regular function

        def get_condition(item):
            return item.condition

        best_score = max(item_list, key=get_condition)

Finally, we could do a manual iteration to locate the item with the max condition in the list!

return(best_score)





def swap_best_by_category(self, other, my_priority, their_priority):
my_item = self.get_best_by_category(their_priority)
their_item = other.get_best_by_category(my_priority)
result = self.swap_items(other, my_item, their_item)

return result
Comment on lines +88 to +93
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Great reuse of all your functions! Part of what lets us write this so concisely is that the error conditions and returns expected by swap_best_by_category are all properly accounted for already in the other functions we are using.


2 changes: 1 addition & 1 deletion tests/integration_tests/test_wave_01_02_03.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
from swap_meet.vendor import Vendor
from swap_meet.item import Item

@pytest.mark.skip
# @pytest.mark.skip
@pytest.mark.integration_test
def test_integration_wave_01_02_03():
# make a vendor
Expand Down
2 changes: 1 addition & 1 deletion tests/integration_tests/test_wave_04_05_06.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
from swap_meet.decor import Decor
from swap_meet.electronics import Electronics

@pytest.mark.skip
# @pytest.mark.skip
@pytest.mark.integration_test
def test_integration_wave_04_05_06():
camila = Vendor()
Expand Down
20 changes: 11 additions & 9 deletions tests/unit_tests/test_wave_01.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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"
Expand All @@ -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(
Expand All @@ -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(
Expand All @@ -49,7 +49,9 @@ 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 **********
# *********************************************************************


# with pytest.raises(TypeError):
assert result == False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

And since the description says this should literally return False, I like the explicit False check rather than a more generic falsy check.



18 changes: 14 additions & 4 deletions tests/unit_tests/test_wave_02.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@
from swap_meet.vendor import Vendor
from swap_meet.item import Item

@pytest.mark.skip
# @pytest.mark.skip
def test_items_have_blank_default_category():
item = Item()
assert item.category == ""

@pytest.mark.skip
# @pytest.mark.skip
def test_get_items_by_category():
item_a = Item(category="clothing")
item_b = Item(category="electronics")
Expand All @@ -23,7 +23,7 @@ def test_get_items_by_category():
assert item_c in items
assert item_b not in items

@pytest.mark.skip
# @pytest.mark.skip
def test_get_no_matching_items_by_category():
item_a = Item(category="clothing")
item_b = Item(category="clothing")
Expand All @@ -33,8 +33,18 @@ def test_get_no_matching_items_by_category():
)

items = vendor.get_by_category("electronics")

# assert len(items) == 0
# assert item_a in items
# assert item_c in items
# assert item_b not in items

raise Exception("Complete this test according to comments below.")
assert len(items) == 0
assert item_a != Item(category="clothing")
assert item_b != Item(category="clothing")
assert item_c != Item(category="clothing")
Comment on lines +43 to +45
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These checks aren't necessary. In fact they will always be true. One thing we haven't dwelled on too much is value equality versus reference equality. We've seen that we can compare the contents of say lists or dicts using == which does go through and compare the value of each piece of data in those collections. We can also compare to instance's ids, or use the is operator, which also compares their references (ids). For example [] == [] is true (they are both empty lists), but [] is [] is false (they are two different lists, both of which happen to be empty).

For any class we write, the default way that Python compares them with == is reference equality. So since each of those checks is comparing a previously existing instance to a newly constructed instance, all of them will compare as not equal.

We can research the __eq__ method if we'd like to provide our own value comparison logic.



# *********************************************************************
# ****** Complete Assert Portion of this test **********
# *********************************************************************
12 changes: 6 additions & 6 deletions tests/unit_tests/test_wave_03.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@
from swap_meet.vendor import Vendor
from swap_meet.item import Item

@pytest.mark.skip
# @pytest.mark.skip
def test_item_overrides_to_string():
item = Item()

stringified_item = str(item)

assert stringified_item == "Hello World!"

@pytest.mark.skip
# @pytest.mark.skip
def test_swap_items_returns_true():
item_a = Item(category="clothing")
item_b = Item(category="clothing")
Expand Down Expand Up @@ -38,7 +38,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(category="clothing")
item_b = Item(category="clothing")
Expand All @@ -65,7 +65,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(category="clothing")
item_b = Item(category="clothing")
Expand All @@ -92,7 +92,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=[]
Expand All @@ -112,7 +112,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(category="clothing")
item_b = Item(category="clothing")
Expand Down
Loading