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

Sea Turtles - Esther Annorzie #106

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Sea Turtles - Esther Annorzie #106

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
fa75748
passed wave 1 unit tests
estherannorzie Apr 4, 2022
6e8bfbc
passed wave 2 unit tests
estherannorzie Apr 5, 2022
6a33677
finished first test in wave 3
estherannorzie Apr 6, 2022
a928b5e
passed wave 3 unit tests
estherannorzie Apr 6, 2022
8e4a74d
added main.py
estherannorzie Apr 6, 2022
12a0b1b
passed wave 4 unit tests
estherannorzie Apr 6, 2022
98c1c35
passed 4/5 wave 5 unit tests
estherannorzie Apr 6, 2022
5efb5df
passed wave 5 unit tests
estherannorzie Apr 6, 2022
b18eae7
passed first three wave 6 tests
estherannorzie Apr 6, 2022
6e65688
added some wave 6 assertions
estherannorzie Apr 7, 2022
f95379f
fixed some class inheritance issues
estherannorzie Apr 7, 2022
ed3141d
refactored tests 1-3 in wave 6
estherannorzie Apr 7, 2022
d90cb23
passed more 4/8 wave 6 tests
estherannorzie Apr 7, 2022
5439fd7
passed all tests
estherannorzie Apr 7, 2022
3612871
refactored swap_best_by_category
estherannorzie Apr 8, 2022
c813457
added docstrings
estherannorzie Apr 8, 2022
7d074af
refactored based on instructor feedback
estherannorzie Apr 15, 2022
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
3 changes: 3 additions & 0 deletions main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
from swap_meet.item import Item
from swap_meet.vendor import Vendor

10 changes: 8 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,8 @@
class Clothing:
pass
from .item import Item

class Clothing(Item):
def __init__(self, category="", condition=0):
super().__init__("Clothing", condition)
Copy link

@kelsey-steven-ada kelsey-steven-ada Apr 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

For all of the subclasses of Item, we could remove the category parameter from the child's __init__ method since we don't want folks to change that value. We always want a Clothing object to have the category "Clothing", so we can decide to not even let someone pass a different value.

Suggested change
def __init__(self, category="", condition=0):
super().__init__("Clothing", condition)
def __init__(self, condition=0):
super().__init__("Clothing", condition)


def __str__(self):
return "The finest clothing you could wear."
10 changes: 8 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,8 @@
class Decor:
pass
from .item import Item

class Decor(Item):
def __init__(self, category="", condition=0):
super().__init__("Decor", condition)

def __str__(self):
return "Something to decorate your space."
10 changes: 8 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,8 @@
class Electronics:
pass
from .item import Item

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

def __str__(self):
return "A gadget full of buttons and secrets."
21 changes: 20 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,21 @@
class Item:
pass
def __init__(self, category="", condition=0):
self.category = category
self.condition = condition

def __str__(self):
return "Hello World!"

def condition_description(self):
if not self.condition:
return "This item has been heavily used."
elif self.condition == 1:
return "This item has been moderately used."
elif self.condition == 2:
return "This item has been partially used."
elif self.condition == 3:
return "This item is in good condition."
elif self.condition == 4:
return "This item is in great condition."
elif self.condition == 5:
return "This item is brand new!"
Copy link

Choose a reason for hiding this comment

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

Nice handling for condition! It looks like the code assumes that self.condition will always be an integer, but what if we allowed decimals like a condition of 3.5? How could we adapt our code to convert the decimal to an int we could use here, or alternately, change the if-statements to support decimals?

Another approach for finding the condition description could be to make a dictionary that maps conditions as keys to descriptions as values. There's a trade off of using more memory creating the map variable, but if memory isn't a concern, for me, it's a little easier to read.

Suggested change
if not self.condition:
return "This item has been heavily used."
elif self.condition == 1:
return "This item has been moderately used."
elif self.condition == 2:
return "This item has been partially used."
elif self.condition == 3:
return "This item is in good condition."
elif self.condition == 4:
return "This item is in great condition."
elif self.condition == 5:
return "This item is brand new!"
condition_map = {
0: "This item has been heavily used.",
1: "This item has been moderately used.",
2: "This item has been partially used.",
3: "This item is in good condition.",
4: "This item is in great condition.",
5: "This item is brand new!"
}
return condition_map[self.condition]

125 changes: 124 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,125 @@
class Vendor:
pass
def __init__(self, inventory=None):
if not inventory:
inventory = []
self.inventory = inventory
Copy link

@kelsey-steven-ada kelsey-steven-ada Apr 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Nice handling of the None default value! Another way we could write this is with a ternary operator:

Suggested change
if not inventory:
inventory = []
self.inventory = inventory
self.inventory = inventory if inventory else []

This can be read as "If the parameter inventory is truthy, assign self.inventory to inventory, else set self.inventory to an empty list". The general blueprint for a ternary operator looks like: variable_name = <value_if_condition_true> if <condition_statement> else <value_if_condition_false>.
More info: https://book.pythontips.com/en/latest/ternary_operators.html


def add(self, item):
"""Adds an item to the instance's inventory and returns it."""
self.inventory.append(item)
return item

def remove(self, item):
"""
Removes items from the instance's inventory.

Parameter:
item (str): Inventory item to be removed.

Returns:
bool: True if item is inventory, False otherwise.
Copy link

Choose a reason for hiding this comment

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

Here's something we need to watch out for with comments: on line 20, the comment says the function returns True if the item is in the inventory, but the code below returns the Item that was removed in that circumstance.

It can be confusing for folks using our code when comments and code are out of sync - a user or another coder might not know which is the desired behavior of the function! It's a good practice to do a final review of comments across a file as part of our clean up process when we think we're about done.

"""

if item in self.inventory:
self.inventory.remove(item)
return item
return False
Copy link

Choose a reason for hiding this comment

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

I like inverting the logic of the condition to be able to treat it like a guard clause, which allows us to outdent the main logic of the function something like the following:

        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.


def get_by_category(self, specific_category):
"""
Adds the items in the instance's inventory that are of a specific category.
Copy link

Choose a reason for hiding this comment

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

When writing a high-level description of what a function does, I suggest focusing on the observable changes for users and being specific. Here, it's unclear what "Adds the items..." means. I have the context to know it means adding items to a list, but to a user with no information about how the code works, this comment could be read to mean that we are literally adding Items together by performing some kind of arithmetic on them. If we replace "Adds" with something like "Gets" or "Collects", it becomes clearer that the function is grouping together Items.


Parameters:
specific_category (str): A category.

Returns:
category_matches (list): Items in the inventory matching the category.
"""

category_matches = []
for item in self.inventory:
if item.category == specific_category:
category_matches.append(item)
Copy link

@kelsey-steven-ada kelsey-steven-ada Apr 12, 2022 •
edited
Loading

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

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

Which here would look like:

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

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

return category_matches

def swap_items(self, friend, my_item, their_item):
"""
Swaps items in vendor's inventory and friend's inventory.

Parameters:
friend (class): Vendor instance of friend.
my_item (str): Category the vendor wants to receive.
their_item (str): Category friends wants to receive.

Returns:
(bool): True if swapping occurs, False if not.
"""

if my_item in self.inventory and their_item in friend.inventory:
friend.inventory.append(my_item)
self.inventory.remove(my_item)
Copy link

Choose a reason for hiding this comment

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

I love how easy this is to read. We could try to condense this down to a single line using the pop() function, but I think we'd lose clarity and ease of understanding the code:

# Slightly less code, but also less clear about what's going on
friend.inventory.append(self.inventory.pop(0))


self.inventory.append(their_item)
friend.inventory.remove(their_item)
return True
return False

def swap_first_item(self, friend):
'''
Removes first item from a vendor's inventory and a friend's inventory.

Parameter:
friend (class): An instance of another Vendor.

Returns:
(bool): True is first items the Vendor and the friend swap items, False otherwise.
'''
if self.inventory and friend.inventory:
friend.inventory.append(self.inventory[0])
self.inventory.remove(self.inventory[0])

self.inventory.append(friend.inventory[0])
friend.inventory.remove(friend.inventory[0])
return True
Copy link

Choose a reason for hiding this comment

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

We could reuse the code you wrote to swap items here! 😄 Both the solution above and reusing the swap_items function winds up being O(n), mostly due to removing the items from the inventory.

Suggested change
friend.inventory.append(self.inventory[0])
self.inventory.remove(self.inventory[0])
self.inventory.append(friend.inventory[0])
friend.inventory.remove(friend.inventory[0])
return True
my_item = self.inventory[0]
friend_item = friend.inventory[0]
return self.swap_items(friend, my_item, friend_item)

We could also swap the items in place to have O(1) runtime with something like:

self.inventory[0], friend_vendor.inventory[0] = friend_vendor.inventory[0], self.inventory[0]

return False

def get_best_by_category(self, specific_category):
'''
Returns the item with the best condition in a specific category.

Parameters:
specific_category (str): Category.

Returns:
(instance or bool): Returns instance of an item in a specfic category's best condition or None if not found.
'''
best_by_category = None
highest_condition = 0

for item in self.inventory:
if item.category == specific_category and item.condition > highest_condition:
Copy link

Choose a reason for hiding this comment

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

This line is a bit long, we could break it over multiple lines between the logic statements to make it easier to read:

Suggested change
if item.category == specific_category and item.condition > highest_condition:
if (item.category == specific_category and
item.condition > highest_condition):

best_by_category = item
highest_condition = item.condition
return best_by_category

def swap_best_by_category(self, other, my_priority, their_priority):
'''
Determines if vendor's item priority and other's item priority exists and swaps items.

Parameters:
other (class): The other Vendor instance.
my_priority (str): The category that the Vendor wants to receive.
their_priority (str): The category that the other party wants to receive.

Returns:
(bool): True if items are swapped, False is not.
'''

their_best_item = self.get_best_by_category(their_priority)
my_best_item = other.get_best_by_category(my_priority)
Comment on lines +121 to +122
Copy link

Choose a reason for hiding this comment

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

Great code reuse!


if not their_best_item or not my_best_item:
return False

swapped_items = self.swap_items(other, their_best_item, my_best_item)
Copy link

Choose a reason for hiding this comment

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

swapped_items sounds like it might hold Item objects. I'd consider variable names like are_items_swapped or did_swap_items to more clearly represent that this is a boolean value.

return swapped_items
Copy link

Choose a reason for hiding this comment

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

Since we don't perform other actions with swapped_items before we return it, we could remove the variable assignment and directly return the value from calling self.swap_items(...):

Suggested change
swapped_items = self.swap_items(other, their_best_item, my_best_item)
return swapped_items
return self.swap_items(other, their_best_item, my_best_item)

4 changes: 2 additions & 2 deletions tests/integration_tests/test_wave_01_02_03.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
from swap_meet.vendor import Vendor
from swap_meet.item import Item

@pytest.mark.skip
@pytest.mark.integration_test
# @pytest.mark.skip
# @pytest.mark.integration_test
Copy link

Choose a reason for hiding this comment

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

We want to comment out the @pytest.mark.skip decorator so the test will run, but we should leave the @pytest.mark.integration_test decorator active for all the tests in the project that use it.

The integration_test decorator directs pytest to run these tests after all of our unit tests. We want to do this because unit tests are small and isolated, making them more helpful for debugging issues. Our integration tests are larger, checking if multiple pieces are working together, which can make it harder to tell where an error came from, if one arises.

def test_integration_wave_01_02_03():
# make a vendor
vendor = Vendor()
Expand Down
4 changes: 2 additions & 2 deletions tests/integration_tests/test_wave_04_05_06.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
from swap_meet.decor import Decor
from swap_meet.electronics import Electronics

@pytest.mark.skip
@pytest.mark.integration_test
# @pytest.mark.skip
# @pytest.mark.integration_test
def test_integration_wave_04_05_06():
camila = Vendor()
valentina = Vendor()
Expand Down
15 changes: 9 additions & 6 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,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,10 @@ 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 len(vendor.inventory) == 3
assert item not in vendor.inventory
assert result == False
Comment on lines +56 to +58
Copy link

Choose a reason for hiding this comment

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

Nice checks for all the relevant data!

12 changes: 8 additions & 4 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 @@ -34,7 +34,11 @@ def test_get_no_matching_items_by_category():

items = vendor.get_by_category("electronics")

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 len(items) == 0
assert item_a not in items
assert item_b not in items
assert item_c not in items
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
Loading