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

Spruce - Reid #83

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
11 changes: 9 additions & 2 deletions swap_meet/clothing.py
Original file line number Diff line number Diff line change
@@ -1,2 +1,9 @@
class Clothing:
pass
from swap_meet.item import Item

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

Choose a reason for hiding this comment

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

If we added support for the condition attribute to Item itself (and updated the constructor to match, see below) then we could make use of the constructor argument to pass through the condition parameter here.

        super().__init__(category="Clothing", condition=condition)

Also, good call on removing category from the Clothing constructor. We don't want to allow the creation of a Clothing instance for which the category isn't "Clothing". (Of course, since this is Python, we could simply reassign it later on. But there are other Python features that could help us prevent that if we really needed to!)


def __str__(self):
return "The finest clothing you could wear."
10 changes: 8 additions & 2 deletions swap_meet/decor.py
Original file line number Diff line number Diff line change
@@ -1,2 +1,8 @@
class Decor:
pass
from swap_meet.item import Item

class Decor(Item):
def __init__(self, condition = 0):
super().__init__(category = "Decor")
self.condition = condition
Comment on lines +5 to +6

Choose a reason for hiding this comment

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

The same situation applies here as for the Clothing constructor.

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

class Electronics(Item):
def __init__(self, condition = 0):
super().__init__(category = "Electronics")
self.condition = condition
Comment on lines +5 to +6

Choose a reason for hiding this comment

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

The same situation applies here as for the Clothing constructor.

def __str__(self):
return "A gadget full of buttons and secrets."
32 changes: 31 additions & 1 deletion swap_meet/item.py
Original file line number Diff line number Diff line change
@@ -1,2 +1,32 @@
from swap_meet.vendor import Vendor

class Item:
pass
def __init__(self, category = ""):
self.category = category
Comment on lines +4 to +5

Choose a reason for hiding this comment

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

Notice this requirement in the project description:

All three classes and the Item class have an attribute called condition, which can be optionally provided in the initializer. The default value should be 0.

Rather than each of the child classes defining it condition on its own, we can add the condition to the Item constructor, like

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


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

def condition_description(self):

Choose a reason for hiding this comment

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

Good job putting this implementation in the parent class, but also notice that this method is expecting the instance to have an instance member called condition, but currently Item doesn't have that. So if we created an Item and then tried calling its condition_description method, Python would crash with an AttributeError. The child classes derived from Item do have this attribute (although, think about what would happen if we forgot to in one), but in general we shouldn't make assumptions about a class having attributes that aren't defined in the class itself (although this can be used for a pattern called the Template Method pattern). So trying to access the condition method here in Item should also give us a hint that initializing the condition in our constructor is a good idea.

if self.condition == 1:
return "Ew."
elif self.condition == 2:
return "Um."
elif self.condition == 3:

Choose a reason for hiding this comment

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

What would happen if we tried to get the description for an Item with a condition of 3.5 (there are tests that set the condition to 3.5, but don't try to read the description). The description test checks for a condition of 1 and a description of 5, but we should still make sure our code covers any other reasonable conditions, including floating point values (since we've seen them used in other tests).

How could we modify this (slightly) to cover the values between the whole numbers?

return "I mean, I guess."
elif self.condition == 4:
return "Acceptable."
elif self.condition == 5:
return "Yes."












Comment on lines +21 to +32

Choose a reason for hiding this comment

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

Mostly a nit pick, but try to not leave big chunks of unnecessary whitespace in your files.

78 changes: 77 additions & 1 deletion swap_meet/vendor.py
Original file line number Diff line number Diff line change
@@ -1,2 +1,78 @@
class Vendor:
pass
def __init__(self, inventory = None):
if inventory is None:
self.inventory = []
else:
self.inventory = inventory
Comment on lines +3 to +6

Choose a reason for hiding this comment

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

Nice handling of the default empty inventory.

Notice that when we have the pattern

if some_condition:
    my_var = "some value"
else:
    my_var = "other value"

This can be re-written as a ternary, as:

my_var = "some value" if some_condition else "other value"

In this situation, that would look like

        self.inventory = [] if inventory is None else inventory


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

def remove(self, item_to_remove):
if item_to_remove not in self.inventory:
return False
Comment on lines +13 to +14

Choose a reason for hiding this comment

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

Nice guard clause here, which allowed us to unindent the remainder of the function after handling this case! We need to do this check since trying to remove an item from a lis that isn't in the list results in a ValueError.

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.

self.inventory.remove(item_to_remove)
return item_to_remove

def get_by_category(self, category):
category_matches = []
for item in self.inventory:
if item.category == category:
category_matches.append(item)
Comment on lines +19 to +22

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_matches = [item for item in self.inventory if item.category == category]

return category_matches

def swap_items(self, other_vendor, selfs_item, others_item):
if selfs_item in self.inventory and others_item in other_vendor.inventory:

Choose a reason for hiding this comment

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

I would reverse the sense of this comparison to check for the items not being in the lists, and returning False, acting as a guard clause (like you did in the remove method). Reversing the comparison can be helpful if we see the else part of our conditional being very short (especially if it's a single return statement).

self.remove(selfs_item)
self.add(others_item)

other_vendor.remove(others_item)
other_vendor.add(selfs_item)

return True

else:
return False

def swap_first_item(self, other_vendor):
if (len(self.inventory) == 0) or (len(other_vendor.inventory) == 0):
return False
else:
Comment on lines +39 to +41

Choose a reason for hiding this comment

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

It's a little more pythonic to check for an empty list by taking advantage of its falsiness rather than explicitly checking the len(), as in

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

Also, notice that since the only thing you're doing in the positive condition is to return, this is essentially a guard clause. We can leave off the else and unindent the lines that were within it.

selfs_item = self.remove(self.inventory[0])
others_item = other_vendor.remove(other_vendor.inventory[0])

self.add(others_item)
other_vendor.add(selfs_item)

return True

def get_best_by_category(self, category = ""):

Choose a reason for hiding this comment

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

The description of this function didn't ask for a default value for the category parameter. And arguably, it makes sense to require the caller to provide a value. Since the function is called get_best_by_category, I would expect to need to have a particular category in mind, and it might read a little strangely to see a call like some_vendor.get_best_by_category() where the category isn't stated.

best_condition = -1
best_item = None
for item in self.inventory:
if ((item.category == category) and
(item.condition > best_condition)):
best_item = item
best_condition = item.condition
Comment on lines +53 to +57

Choose a reason for hiding this comment

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

This logic is doing two things: 1. restricting to items of a certain category, and 2. looking for the best condition. Do we have a method that filters to a category that we could reuse here?

return best_item

def swap_best_by_category(self, other = None, my_priority = "", their_priority = ""):

Choose a reason for hiding this comment

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

The description of this function didn't ask for a default value for the parameters. And in cases where a default is supplied, it's a little surprising that it can lead to an error. Notice that if None is used for other, line 62 will crash when it tries to access get_best_by_category for a NoneType. If instead we lead off the default values, then if we tried to call the method without the needed parameters we would get an error about some required parameters being missing.

self_best = self.get_best_by_category(their_priority)
other_best = other.get_best_by_category(my_priority)

if not self_best or not other_best:
return False
else:
self.swap_items(other, self_best, other_best)
return True
Comment on lines +64 to +68

Choose a reason for hiding this comment

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

Because of how swap_items is implemented, the empty checks here aren't necessary. So long as we make use of the return value from swap_items, we could get away with

        return self.swap_items(other, self_best, other_best)