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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍 We need to import Item since we use it by name in the Clothing declaration.

import uuid

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We don't need to import uuid. uuid isn't referenced in this file. Item needs it so that it can perform its default logic for the id parameter, but we don't need it here.


class Clothing(Item):
def __init__(self, id=None, fabric="Unknown", condition=0):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👀 It's usually preferred to follow the same order of parameters as used by the parent type for the parameters that are shared in common

    def __init__(self, id=None, condition=0, fabric="Unknown"):

super().__init__(id, condition)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍 Using Item's __init__ gives Item a chance to do any initialization it needs to in order to function properly. In this small example, it doesn't do much (only sets the id and condition), but in more complex classes, it could be doing a lot more.

self.fabric = fabric

def get_category(self):
return "Clothing"
Comment on lines +9 to +10

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👀 Because your implementation of get_category() in Item uses introspection to determine the name of the class rather than using a hard coded "Item" string, we don't need this overridden method. The implementation in Item already works properly for derived types.

Why might that be?


def __str__(self):
return f"An object of type Clothing with id {self.id}. It is made from {self.fabric} fabric."

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Notice that the first part of the description here is the same as what Item generates as a string, but with "Clothing" in the string. If Item's __str__ made use of get_category, it would insert the property category name, and then we could make use of Item's string logic by calling super().__str__() and concatenating the Clothing-specific details.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As written, this line is getting a bit long. Consider wrapping the line.

        return (
            f"An object of type Clothing with id {self.id}. "
            f"It is made from {self.fabric} fabric."
        )

Alternatively, reusing the description from Item would also help shorten this.


17 changes: 15 additions & 2 deletions swap_meet/decor.py
Original file line number Diff line number Diff line change
@@ -1,2 +1,15 @@
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, condition)
self.width = width
self.length = length

def get_category(self):
return "Decor"

def __str__(self):
return f"An object of type Decor with id {self.id}. It takes up a {self.width} by {self.length} sized space."

17 changes: 15 additions & 2 deletions swap_meet/electronics.py
Original file line number Diff line number Diff line change
@@ -1,2 +1,15 @@
class Electronics:
pass
from swap_meet.item import Item

class Electronics(Item):
def __init__(self, id=None, type="Unknown", condition=0):
super().__init__(id, condition)
self.type = type


def get_category(self):
return "Electronics"

def __str__(self):
return f"An object of type Electronics with id {self.id}. This is a {self.type} device."


27 changes: 26 additions & 1 deletion swap_meet/item.py
Original file line number Diff line number Diff line change
@@ -1,2 +1,27 @@
import uuid

class Item:
pass
def __init__(self, id=None, condition=0):
self.id = uuid.uuid4().int if id is None else id

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍 Good use of a sentinel check to generate a new id. We can't just put the uuid.uuid4().int as the default value, since it would only generate a single id that would be shared as the value for all instances that needed a default.

self.condition = float(condition)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There's no particular need to force this to be a float is all we're expecting is numerical types. Including this call would allow passing a string representation of a float (e.g., "3.5") successfully, but it's not our job to accept every possible kind of input, and trying to do so can have unexpected results. Like here, I could also successfully pass in True or False as the condition, and each can be converted to a float. Would we want to allow this?


def get_category(self):
return type(self).__name__

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍 All we were really looking for folks to do here was to return "Item", and then later override this function for each subtype. This approach avoids needing to override the behavior by using the introspection/reflection features of Python to get the class for the current instance (which will be the derived class type if this were called on an instance of a subclass). And then reads the name that class was given when it was defined.

Be sure that you have a clear idea of how this works for the subclasses.


def __str__(self):
return f"An object of type {self.get_category()} with id {self.id}."

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍 Using get_category() means that we could potentially reuse this as part of the string output of derived types, since get_category() will behave "correctly" for child classes.


def condition_description(self):
if self.condition == 5:
return "Brand New"
elif self.condition == 4:
return "Like New"
elif self.condition == 3:
return "Lightly Used"
elif self.condition == 2:
return "Moderately Used"
elif self.condition == 1:
return "Heavily Used"
Comment on lines +15 to +24

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Writing explicit checks works, but has some drawbacks. In the general case, there could be an arbitrary number of conditions, and we would have to write more code to cover those conditions. What would this code be like if there we 10 condition rankings? 100?

Notice how this is very similar to our snowman drawing code, where initially we picked a string to draw based on some input, but then later, we refactored it by moving those strings into a list, and using the input as an index into the list. So we could think about doing the same thing here.

To get the maximum benefit, we would want to declare the description data structure outside the function (just like snowman did), so that it only gets initialized a single time, rather than every time the function is called.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

While we don't have a test that tries to get the condition description for a floating point condition, we do check that we can make an Item with floating point conditions. What would this logic do if we tried to get the description for an Item having a condition of 3.5?

In the case where we move the description strings into a data structure, how could we provide a useful behavior for values between the integer conditions?




98 changes: 97 additions & 1 deletion swap_meet/vendor.py
Original file line number Diff line number Diff line change
@@ -1,2 +1,98 @@
from swap_meet.item import Item
class Vendor:
pass
def __init__(self, inventory=None):
self.inventory = [] if inventory is None else inventory

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍 We should avoid mutable values as default parameters. So if we need to setup of a default mutable value for a class, we use a default sentinel immutable value (a specific value we can look for), and if we see it, create a new mutable instance.


def add(self, item):
self.inventory.append(item)
return item

def remove(self, item):
if item in self.inventory:
self.inventory.remove(item)
return item
return False
Comment on lines +11 to +14

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍 We need to check the item is actually there before using remove, or else we would need to handle the error it can raise.

Consider reordering the statements as

        if item not in self.inventory:
            return False

        self.inventory.remove(item)
        return item

which allows us to emphasize the important code by unindenting and placing it in a more significant part of the function, rather than being just one branch of a condition.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

One minor quibble could be made that this is returning the object instance that was passed to the function rather than the instance that was found in the inventory. If we really want to be sure that we're returning the one from the inventory, we could use index to find its location (which would raise an IndexError if it wasn't found). Then we could return the result of popping the value. This would give us the actual instance from the list.

In practice, with how we implement Item, this isn't necessary, but if we provided a custom equality check, then it might. Also, for the wave 1 case where we're using strings, it's possible that the search instance and the stored instance of a string could be two different strings.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I know the spec says this should return False when the item is not found, but personally. I woud prefer returning None (or even just raising a ValueError!).


# Wave_2

def get_by_id(self, id):
for item in self.inventory:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍 We need to iterate through the list to find the object with the matching id.

if id == item.id:
return item
return None

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Consider adding a blank line before this return to help visually seperate it from the loop.



#wave_3

def swap_items(self, other_vendor, my_item, their_item):

if not my_item in self.inventory or not their_item in other_vendor.inventory:
return False
Comment on lines +29 to +30

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍 We do need to check that both items are in their respective list before starting to remove things, or else we could end up in a situation where we start updating (removing from) one list, but then find that the other item is missing, leaving us with a partial change we don't want.



self.remove(my_item) #removes `my_item` from this `Vendor`'s inventory

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nit: PEP8 recommends starting "inline" comments at least two spaces after the code part, and there should always be a space between the # and the comment

        self.remove(my_item)  # removes `my_item` from this `Vendor`'s inventory

other_vendor.add(my_item) #adds it to the friend's inventory
self.add(their_item) #adds it to this `Vendor`'s inventory
other_vendor.remove(their_item) #removes `their_item` from the other `Vendor`'s inventory

return True


# Wave_4

def swap_first_item(self, other_vendor):

if not self.inventory or not other_vendor.inventory:
return False
Comment on lines +45 to +46

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍 Trying to get position 0 from an empty inventory would be an error.


# Remove the first item from both vendors' inventories
self_first_item = self.inventory.pop(0)
others_first_item = other_vendor.inventory.pop(0)

# Add the first item from the other vendor to each vendor's inventory
self.inventory.insert(0, others_first_item)
other_vendor.inventory.insert(0,self_first_item)
Comment on lines +48 to +54

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Notice that both pop and insert (both at position 0) are linear time complexity, since first with the pop everything after position 0 needs to be shifted forward to fill the removed item. Then to insert at position 0, everything must be shifted back to make room for the item.

For the insert part, while we are swapping the items that started as the first thing in the list, there was no explicit requirement that they need to end up in the starting position (and the tests don't enforce this). So we could use append to add the items at the end, avoiding the linear cost for insert. However, we still have the linear cost for pop.

To work around the linear cost for pop, rather than popping directly from position 0, we could exchange the item reference with the reference at the end of the list (constant time) and then pop from the end of the list (also constant time).

But if we're thinking about exchanging references, we could exchange the object references between the two inventories directly. Something like

        self_first_item  = self.inventory[0]  # set aside reference to our first item
        self.inventory[0] = other_vendor.inventory[0]  # replace our first item with the other's
        other_vendor.inventory[0] = self_first_item  # replace the other's first item with ours that we set aside

There are more concise ways of writing this (look up tuple swap), but the effect would be the same.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Another approach could be to use our swap_items method to perform the swap (passing the item at position 0 in each inventory as the my and their items). In the general case, swap_items must have linear time complexity, but it would be possible to rewrite swap_items (if we accept that the swapped items needn't wind up at the end of the inventories) so that in the specific case of swapping the first items, it could still have constant time complexity.


# Return True to indicate the swap was successful
return True


# Wave_6
def get_by_category(self, category):
list_of_objects = []
for item in self.inventory:
if category == item.get_category():
list_of_objects.append(item)
return list_of_objects
Comment on lines +62 to +66

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍 We need to loop to find all the items with the specified category.

This would be a great opportunity to try using list comprehension syntax.



def get_best_by_category(self, category):
if not self.get_by_category(category):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍 Nice reuse of the previously written function.

But rather than calling the function multiple times below, save the result once in a variable, and then use that variable. Otherwise, each time the function is called it will need to iterate through the inventory again.

        category_items = self.get_by_category(category)  # then use category_items below
        if not category_items:
            ...

return None

best_item = self.get_by_category(category)[0]
for item in self.get_by_category(category):
if item.condition > best_item.condition:
best_item = item
Comment on lines +73 to +76

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍 Nice logic to loop through and find the item with the best condition.

return best_item

def swap_best_by_category(self, other_vendor, my_priority, their_priority):
if not self.inventory or not other_vendor.inventory:
return False

my_best_item = self.get_best_by_category(their_priority)
their_best_item = other_vendor.get_best_by_category(my_priority)
if not my_best_item or not their_best_item:
return False

if my_best_item.get_category() == their_priority and their_best_item.get_category() == my_priority:
other_vendor.add(my_best_item)
self.remove(my_best_item)
self.add(their_best_item)
other_vendor.remove(their_best_item)
return True
Comment on lines +88 to +93

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👀 Notice that this condition check is unnecessary. If we found an item for each of the get_best_by_category calls, they must already be of the proper category, or they wouldn't have been returned as the best of the desired category.

Once we have the two items to swap, this is the same as swapping any two other items, so we could reuse our swap_items method here.






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
26 changes: 20 additions & 6 deletions tests/unit_tests/test_wave_01.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,13 @@
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 +17,8 @@ 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 +29,8 @@ 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 +43,8 @@ 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 +53,17 @@ def test_removing_not_found_is_false():

result = vendor.remove(item)

raise Exception("Complete this test according to comments below.")
assert result is False

assert len(vendor.inventory) == 3

assert "a" in vendor.inventory
assert "b" in vendor.inventory
assert "c" in vendor.inventory


# raise Exception("Complete this test according to comments below.")
# *********************************************************************
# ****** Complete Assert Portion of this test **********
# *********************************************************************
assert result == False

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This line is redundant with line 65.

Of the two, if we do literally want to compare with one of the boolean constants, we usually use == (and is for None). Technically, == wouldn't guarantee that result was literally False (in Python 0 == False and 1 == True), so I could be persuaded to prefer is since we're expecting to literally return either True or False.

Note that in application code, we rarely if ever compare against one of the boolean constants (with if some_val: preferred for a truthy check, and if not some_val: for a falsy one, and bool(some_val) if we just need to evaluate in a boolean context), but in a test that is meant to confirm the specified behavior, it's acceptable.

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

@pytest.mark.skip
# @pytest.mark.skip
def test_items_have_default_uuid_length_id():
item = Item()
assert isinstance(item.id, int)
assert len(str(item.id)) >= 32

@pytest.mark.skip
# @pytest.mark.skip
def test_item_instances_have_different_default_ids():
item_a = Item()
item_b = Item()
assert item_a.id != item_b.id

@pytest.mark.skip
# @pytest.mark.skip
def test_items_use_custom_id_if_passed():
item = Item(id=12345)
assert isinstance(item.id, int)
assert item.id == 12345

@pytest.mark.skip
# @pytest.mark.skip
def test_item_obj_returns_text_item_for_category():
item = Item()
assert item.get_category() == "Item"

@pytest.mark.skip
# @pytest.mark.skip
def test_get_item_by_id():
test_id = 12345
item_custom_id = Item(id=test_id)
Expand All @@ -36,7 +36,7 @@ def test_get_item_by_id():
result_item = vendor.get_by_id(test_id)
assert result_item is item_custom_id

@pytest.mark.skip
# @pytest.mark.skip
def test_get_item_by_id_no_matching():
test_id = 12345
item_a = Item()
Expand Down
16 changes: 9 additions & 7 deletions tests/unit_tests/test_wave_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
def test_item_overrides_to_string():
test_id = 12345
item = Item(id=test_id)
Expand All @@ -12,7 +12,7 @@ def test_item_overrides_to_string():
expected_result = f"An object of type Item with id {test_id}."
assert item_as_string == expected_result

@pytest.mark.skip
# @pytest.mark.skip
def test_swap_items_returns_true():
item_a = Item()
item_b = Item()
Expand Down Expand Up @@ -40,7 +40,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()
item_b = Item()
Expand All @@ -67,7 +67,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()
item_b = Item()
Expand All @@ -94,7 +94,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 @@ -114,7 +114,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()
item_b = Item()
Expand All @@ -131,7 +131,9 @@ def test_swap_items_from_their_empty_returns_false():

result = fatimah.swap_items(jolie, item_b, nobodys_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 is False

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In addition to checking for the expected return value, since swap_items is known to modify the inventories of the vendors, we should also ensure that in this case, the invetories remain unchanged, similar to the test case you implemented in wave 1.

Loading