All tests pass #112
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,14 @@ | ||
| from swap_meet.item import Item | ||
|
|
||
| class Clothing(Item): | ||
|
|
||
| def __init__(self, condition = 0): | ||
| super().__init__(condition = condition, category = "Clothing") | ||
| self.condition = condition | ||
|
|
||
|
|
||
|
|
||
| def __str__(self): | ||
| self = "The finest clothing you could wear." | ||
| return f"{self}" | ||
|
Comment on lines
+11
to
+13
There was a problem hiding this comment. As with the parent implementation in def __str__(self):
return "The finest clothing you could wear." |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,12 @@ | ||
| from swap_meet.item import Item | ||
|
|
||
| class Decor(Item): | ||
|
There was a problem hiding this comment. The feedback provided about |
||
|
|
||
| def __init__(self, condition = 0): | ||
| super().__init__(condition = condition, category = "Decor") | ||
| self.condition = condition | ||
|
|
||
|
|
||
| def __str__(self): | ||
| self = "Something to decorate your space." | ||
| return f"{self}" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,11 @@ | ||
| from swap_meet.item import Item | ||
|
|
||
| class Electronics(Item): | ||
|
There was a problem hiding this comment. The feedback provided about |
||
|
|
||
| def __init__(self, condition = 0): | ||
| super().__init__(condition = condition, category = "Electronics") | ||
| self.condition = condition | ||
|
|
||
| def __str__(self): | ||
| self = "A gadget full of buttons and secrets." | ||
| return f"{self}" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,23 @@ | ||
|
|
||
| class Item: | ||
| category = "" | ||
|
There was a problem hiding this comment. Assigning variables in the class like this is actually making a class variable, not an instance variable. We only needed instance variables in this project. |
||
|
|
||
| def __init__(self, category = category, condition = 0): | ||
| self.category = category | ||
| self.condition = condition | ||
|
Comment on lines
+5
to
+7
There was a problem hiding this comment. We don't need to use a class variable in the default parameter values here. Strings are immutable, and we can list a string value as a default variable just like we can the integer value for With respect to formatting, keep in mind that PEP8 recommends no spaces around def __init__(self, category="", condition=0):
self.category = category
self.condition = condition |
||
|
|
||
| def __str__(self): | ||
| self = "Hello World!" | ||
| return f"{self}" | ||
|
Comment on lines
+9
to
+11
There was a problem hiding this comment. We shouldn't re-assign def __str__(self):
return "Hello World!" |
||
|
|
||
| def condition_description(self): | ||
|
There was a problem hiding this comment. This logic will handle anything unexpected below the "normal" range (anything 1 or less will be trash), but what would happen for conditions greater than 5? Maybe the behavior here is OK, but be sure to think about those scenarios. 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 self.condition <= 1: | ||
| return "Trash" | ||
| if self.condition <= 2: | ||
| return "Bad" | ||
| if self.condition <= 3: | ||
| return "OK" | ||
| if self.condition <= 4: | ||
| return "Good" | ||
| if self.condition <= 5: | ||
| return "Pristine" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,69 @@ | ||
|
|
||
| from swap_meet.item import Item | ||
|
|
||
| class Vendor: | ||
|
|
||
| def __init__(self, inventory=None): | ||
| if inventory is None: | ||
| inventory = [] | ||
|
Comment on lines
+7
to
+8
There was a problem hiding this comment. Remember that we look at this when the integration tests were failing. When the |
||
| self.inventory = inventory | ||
|
|
||
| def add(self, item): | ||
|
|
||
| self.inventory.append(item) | ||
| return item | ||
|
|
||
| def remove(self, item): | ||
| try: | ||
| self.inventory.remove(item) | ||
| return item | ||
| except Exception: | ||
| return False | ||
|
Comment on lines
+15
to
+20
There was a problem hiding this comment. Great use of error handling to deal with def remove(self, item):
try:
self.inventory.remove(item)
return item
except ValueError:
return False |
||
|
|
||
| def get_by_category(self, category): | ||
| items_by_category = [] | ||
|
|
||
| for item in self.inventory: | ||
| if item.category == category: | ||
| items_by_category.append(item) | ||
|
Comment on lines
+23
to
+27
There was a problem hiding this comment. 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 items_by_category = [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 items_by_category | ||
|
|
||
| def swap_items(self, other_vendor, my_item, their_item): | ||
| if my_item not in self.inventory or their_item not in other_vendor.inventory: | ||
| return False | ||
|
Comment on lines
+32
to
+33
There was a problem hiding this comment. 👍 Great use of a guard clause to make sure that both vendors are in a valid configuration for performing the swap. We could still try to use error handling here, but with the two separate |
||
| self.inventory.remove(my_item) | ||
| other_vendor.inventory.append(my_item) | ||
| other_vendor.inventory.remove(their_item) | ||
| self.inventory.append(their_item) | ||
| return True | ||
|
|
||
| def swap_first_item(self, other_vendor): | ||
| if self.inventory == [] or other_vendor.inventory == []: | ||
| return False | ||
|
|
||
|
|
||
| else: | ||
| self.swap_items(other_vendor, self.inventory[0], other_vendor.inventory[0]) | ||
| return True | ||
|
Comment on lines
+45
to
+47
There was a problem hiding this comment. Since the initial check above returns when the needed conditions (non-empty inventories) aren't met, we can treat it as a guard clause, removing the explicit else, and unindenting the following "main" logic. self.swap_items(other_vendor, self.inventory[0], other_vendor.inventory[0])
return True |
||
|
|
||
|
|
||
| def get_best_by_category(self, category): | ||
|
|
||
| best_condition_so_far = 0 | ||
|
|
||
| list_of_items_by_category = self.get_by_category(category) | ||
|
There was a problem hiding this comment. 👍 Nice function reuse to start off with a list of inventory items filtered to the desired category. |
||
| if list_of_items_by_category == None: | ||
| return None | ||
| for item in list_of_items_by_category: | ||
| if item.condition > best_condition_so_far: | ||
| best_condition_so_far = item.condition | ||
| for item in list_of_items_by_category: | ||
| if item.condition == best_condition_so_far: | ||
| return item | ||
|
Comment on lines
+57
to
+62
There was a problem hiding this comment. We could do these two loops "at once". We could have a second variable that tracks the best item found so far (initialize it to Note that from a big O perspective, what you've written here and what I described are both O(n) with respect to the number of items. Your approach would translate more directly into an approach using |
||
|
|
||
| def swap_best_by_category(self, other, my_priority, their_priority): | ||
|
|
||
| self_best_item = self.get_best_by_category(their_priority) | ||
| their_best_item = other.get_best_by_category(my_priority) | ||
|
|
||
| return self.swap_items(other, self_best_item, their_best_item) | ||
|
Comment on lines
+66
to
+69
There was a problem hiding this comment. 👍 We can reuse several of our existing functions to handle this very cleanly. Remember that |
||
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.
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
Since we're calling the parent's
__init__, which already uses theconditionvalue we pass in to setself.condition, we don't need to set it again ourself.Also following the PEP8 guidelines around keyword and default values, we can rewrite this as