-
Notifications
You must be signed in to change notification settings - Fork 94
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
base: master
Are you sure you want to change the base?
Spruce - Reid #83
Changes from all commits
6ec5e02
a8d484c
ad84d49
868e5c5
7f81b80
98e7780
41cdb60
c16c8c0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
|
||
def __str__(self): | ||
return "The finest clothing you could wear." |
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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." |
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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." |
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Notice this requirement in the project description:
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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
if self.condition == 1: | ||
return "Ew." | ||
elif self.condition == 2: | ||
return "Um." | ||
elif self.condition == 3: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Another approach we could take is to try to remove the item directly, and handle the |
||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 |
||
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 = ""): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 = ""): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because of how return self.swap_items(other, self_best, other_best) |
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
There was a problem hiding this comment.
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 toItem
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.Also, good call on removing category from the
Clothing
constructor. We don't want to allow the creation of aClothing
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!)