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.

Sign up for GitHub

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

Jump to bottom

SeaTurtles_AshleyBenson #110

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

SeaTurtles_AshleyBenson #110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 10 additions & 2 deletions swap_meet/clothing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,10 @@
class Clothing:
pass
#from unicodedata import category
from swap_meet.item import Item

class Clothing(Item):
def __init__(self, condition =0.0):
super().__init__(condition = condition, category = "Clothing")
Comment on lines +5 to +6
Copy link

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 encourage the caller to try to change it. Instead, we pass the literal value that we want to set up to the parent's constructor

With respect to formatting, keep in mind that PEP8 recommends no spaces around = when used for default values or keyword arguments. So we might prefer to write

    def __init__(self, condition=0.0):
        super().__init__(condition=condition, category="Clothing")


def __str__(self):
Copy link

Choose a reason for hiding this comment

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

👍

return "The finest clothing you could wear."

12 changes: 10 additions & 2 deletions swap_meet/decor.py
View file
Open in desktop
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__(condition = condition, category = "Decor")

def __str__(self):
return "Something to decorate your space."

14 changes: 12 additions & 2 deletions swap_meet/electronics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,12 @@
class Electronics:
pass
from multiprocessing import Condition
from swap_meet.item import Item

class Electronics(Item):

def __init__(self, condition = 0 ):
super().__init__(condition = condition, category = "Electronics")

def __str__(self):
return "A gadget full of buttons and secrets."


26 changes: 25 additions & 1 deletion swap_meet/item.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,26 @@
class Item:
pass
def __init__(self, category=None, condition = 0.0):
Copy link

Choose a reason for hiding this comment

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

For default parameters, we only need to be cautious of mutable values (like [] for a vendor's inventory). Strings are immutable, so while your approach definitely works, it would be safe for us to list a string value as the default value directly.

    def __init__(self, category="", condition=0.0):

Also, PEP8 (the python doc about formatting) recommends no spaces around = when used for default parameter values, or named argument calls.

if not category:
category = ""
self.condition = condition
self.category = category


def __str__(self):
Copy link

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

Choose a reason for hiding this comment

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

👍

This works for the values 1, 2, 3, 4, and 5. But what will happen if the condition happens to be outside that range (unexpected, so maybe we don't need to worry about it), or between those values. There's a test that creates Items with condition=3.5. It doesn't check the description, but it deos tell us that we should anticipate needing to handle that situation.

We could modify this to look for ranges, but 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 else 1-5
if self.condition == 1.0:
return "Horrid"
elif self.condition == 2.0:
return "Ugly"
elif self.condition == 3.0:
return "getting warmer"
elif self.condition == 4.0:
return "almost...doesnt count"
elif self.condition == 5.0:
return "winner winner chicken dinner"



81 changes: 80 additions & 1 deletion swap_meet/vendor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,81 @@
class Vendor:
pass
def __init__(self, inventory=None): #define class
if not inventory: #return empty list, if none
self.inventory = [] #attribute set-up
Comment on lines +3 to +4
Copy link

Choose a reason for hiding this comment

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

👍

Nice handling of the need for a mutable default parameter. It would be unsafe to set [] as the default value directly, but we can use None to stand in for the value when not supplied by the caller, and then use a fresh empty list in its place.

In the comment, we're not "return"ing an empty list here. We're using a new empty list when we detect that the placeholder default value was passed in.


else:
self.inventory = inventory #if inv data present

def add(self, item): # a instance method
Copy link

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 item in self.inventory:
self.inventory.remove(item)
return item

else:
return False
Comment on lines +13 to +19
Copy link

Choose a reason for hiding this comment

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

Nice check to make sure the value is there before we try to remove it. Note that we could instead use try/except to handle the case where remove raises an error when the value isn't found.

As written, I'd suggest doing the check for the value not being here first, and then returning as a guard clause, as this allows the main focus of the code to be less indented.

    def remove(self, item):
        if item not in self.inventory:
            return False

        self.inventory.remove(item)
        return item


#a instance method
def get_by_category(self, cat_name):
cat_list = []
for item in self.inventory:
if item.category == cat_name:
cat_list.append(item)
Comment on lines +23 to +26
Copy link

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

        cat_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 cat_list

def swap_items(self, friend, my_item, friend_item):
if my_item in self.inventory and friend_item in friend.inventory:
friend.inventory.append(my_item)
self.inventory.append(friend_item)
friend.inventory.remove(friend_item)
self.inventory.remove(my_item)
return True
return False
Comment on lines +29 to +36
Copy link

Choose a reason for hiding this comment

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

Here again, I would tend to invert the condition to turn this into a guard clause scenario. Compound conditions can be a little tricky to invert, but we could start with:

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

or a little more cleanly as

        # if either vendor doesn't have their needed item
        if my_item not in self.inventory or friend_item not in friend.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.

Then we can write the whole body as

        if my_item not in self.inventory or friend_item not in friend.inventory:
            return False
        
        friend.inventory.append(my_item)
        self.inventory.remove(my_item)
        self.inventory.append(friend_item)
        friend.inventory.remove(friend_item)
        return True


def swap_first_item(self, friend):
if not self.inventory or not friend.inventory:
return False
else:

self.inventory.append(friend.inventory[0])
friend.inventory.append(self.inventory[0])

self.inventory.remove(self.inventory[0])
friend.inventory.remove(friend.inventory[0])
return True
Comment on lines +38 to +48
Copy link

Choose a reason for hiding this comment

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

The way you checked the list lengths works great as a guard clause. Notice that since that check ends in a return, we don't need the literal else, since either we'll have entered the if branch then returned, or we skip it and run the code that follows. This allows us to unindent the stuff under the else (our main code), so that it's a little easier to read.

Also, rather than fetching the 0th item twice, consider getting each once with a local reference, then using that in bot cases. This can minimize the accidental introduction of bugs if we accidentally had an error in the second time we did the lookup. It would also insulate us from the append/remove ordering (once we have a reference to the item, it doesn't matter whether we remove it before or after appending to the other list), which might also help avoid a subtle bug.

    def swap_first_item(self, friend):
        if not self.inventory or not friend.inventory:
            return False

        friend_item = friend.inventory[0]
        my_item = self.inventory[0]
        
        self.inventory.append(friend_item)
        friend.inventory.append(my_item)

        self.inventory.remove(my_item)
        friend.inventory.remove(friend_item)
        return True


def get_best_by_category(self, category):
if not self.inventory:
return None
else:
items = self.get_by_category(category)
if items: return max( items, key=lambda item: item.condition )
Comment on lines +50 to +55
Copy link

Choose a reason for hiding this comment

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

Nice use of max with a key function to help find the item with the "best" condition. We've seen enough python now, that you could try writing your own version of max, including an optional key function parameter. Give it a shot!

Also, since get_by_category returns an empty list if it can't find any items of the desired category, we can do away with the initial empty list check. However, we do need to to either check that thee items result isn't empty before using it with max, or else max will raise an error (or we could handle the error!).

From a style perspective, avoid squeezing if statements and their condition onto a single line. Also, make sure that both branches of the if end up returning something if one of them does. The existing code was correctly returning None as expected when nothing could be found, but this is because of the default Python behavior of returning None when there is no return statement. When one branch is returning something, it looks like an oversight to not return from the other branch, so make sure to explicitly return None if needed (here, rearranged under a guard clause).

    def get_best_by_category(self, category):
        items = self.get_by_category(category)
        if not items:
            return None

        return max( items, key=lambda item: item.condition )




def swap_best_by_category(self, other, my_priority, their_priority):


friend_swap = other.get_best_by_category(my_priority)
my_swap = self.get_best_by_category(their_priority)

if not friend_swap:
return False
if not my_swap:
return False

self.swap_items(other, my_swap, friend_swap)
return True
Comment on lines +59 to +71
Copy link

Choose a reason for hiding this comment

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

👍

Really nice function reuse. Notice that swap_items already shares the same return behavior as we would like here (and would properly handle None being passed in as either param; convince yourself why this is so). So we could even condense this down to

    def swap_best_by_category(self, other, my_priority, their_priority):
        friend_swap = other.get_best_by_category(my_priority)
        my_swap = self.get_best_by_category(their_priority)
        return self.swap_items(other, my_swap, friend_swap)



# if their_priority not in self.inventory:
# return False
# elif my_priority not in other.inventory:
# return False
Comment on lines +74 to +77
Copy link

Choose a reason for hiding this comment

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

Be sure to clean up old commented out code





18 changes: 11 additions & 7 deletions tests/unit_tests/test_wave_01.py
View file
Open in desktop
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,11 +27,11 @@ 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(
inventory=["a", "b", "c", item]
inventory=["a", "b", "c", "item"]
Copy link

Choose a reason for hiding this comment

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

This test was failing.

item here is the variable initialized on line 32 (the string "item to remove"). Adding quotes around this causes the test to fail, since rather than adding "item to remove" to the list, and removing it later, it adds "item" to the list, which then doesn't match the act or assert statements as written.

Restoring this line back to item, the test passes.

)

result = vendor.remove(item)
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,11 @@ def test_removing_not_found_is_false():

result = vendor.remove(item)

raise Exception("Complete this test according to comments below.")
assert len(vendor.inventory) == 3
assert item not in vendor.inventory
assert result == False
Comment on lines +52 to +54
Copy link

Choose a reason for hiding this comment

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

👍
I like the length test to ensure that all the items are still there. We don't really need the check for item not being in the list, since it was never in the list. And I like the explicit False check here, rather than something a little more falsy.

We could still consider checking that each of the three things that were in the list at the start are still there (that there were no shenanigans from the remove), but given that you're checking the overall length already, that's largely a matter of personal preference.


#raise Exception("Complete this test according to comments below.")
# *********************************************************************
# ****** Complete Assert Portion of this test **********
# *********************************************************************
10 changes: 5 additions & 5 deletions tests/unit_tests/test_wave_02.py
View file
Open in desktop
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,8 @@ def test_get_no_matching_items_by_category():
)

items = vendor.get_by_category("electronics")

raise Exception("Complete this test according to comments below.")
assert len(items) == 0
Copy link

Choose a reason for hiding this comment

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

👍

#raise Exception("Complete this test according to comments below.")
# *********************************************************************
# ****** Complete Assert Portion of this test **********
# *********************************************************************
12 changes: 6 additions & 6 deletions tests/unit_tests/test_wave_03.py
View file
Open in desktop
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
6 changes: 3 additions & 3 deletions tests/unit_tests/test_wave_04.py
View file
Open in desktop
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
def test_swap_first_item_returns_true():
item_a = Item(category="clothing")
item_b = Item(category="clothing")
Expand Down Expand Up @@ -30,7 +30,7 @@ def test_swap_first_item_returns_true():
assert item_a in jolie.inventory
assert result

@pytest.mark.skip
#@pytest.mark.skip
def test_swap_first_item_from_my_empty_returns_false():
fatimah = Vendor(
inventory=[]
Expand All @@ -48,7 +48,7 @@ def test_swap_first_item_from_my_empty_returns_false():
assert len(jolie.inventory) == 2
assert not result

@pytest.mark.skip
#@pytest.mark.skip
def test_swap_first_item_from_their_empty_returns_false():
item_a = Item(category="clothing")
item_b = Item(category="clothing")
Expand Down
10 changes: 5 additions & 5 deletions tests/unit_tests/test_wave_05.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,25 +3,25 @@
from swap_meet.decor import Decor
from swap_meet.electronics import Electronics

@pytest.mark.skip
#@pytest.mark.skip
def test_clothing_has_default_category_and_to_str():
cloth = Clothing()
assert cloth.category == "Clothing"
assert str(cloth) == "The finest clothing you could wear."

@pytest.mark.skip
#@pytest.mark.skip
def test_decor_has_default_category_and_to_str():
decor = Decor()
assert decor.category == "Decor"
assert str(decor) == "Something to decorate your space."

@pytest.mark.skip
#@pytest.mark.skip
def test_electronics_has_default_category_and_to_str():
electronics = Electronics()
assert electronics.category == "Electronics"
assert str(electronics) == "A gadget full of buttons and secrets."

@pytest.mark.skip
#@pytest.mark.skip
def test_items_have_condition_as_float():
items = [
Clothing(condition=3.5),
Expand All @@ -31,7 +31,7 @@ def test_items_have_condition_as_float():
for item in items:
assert item.condition == pytest.approx(3.5)

@pytest.mark.skip
#@pytest.mark.skip
def test_items_have_condition_descriptions_that_are_the_same_regardless_of_type():
items = [
Clothing(condition=5),
Expand Down
Loading