Sapphire - Monica Lagdaan#90
Conversation
| @pytest.mark.skip | ||
| @pytest.mark.integration_test | ||
| #@pytest.mark.skip | ||
| #@pytest.mark.integration_test |
There was a problem hiding this comment.
[nit] You should leave the @pytest.mark.integration_test marks
| from swap_meet.electronics import Electronics | ||
|
|
||
| @pytest.mark.skip | ||
| #@pytest.mark.skip |
There was a problem hiding this comment.
[nit] You can delete these vs. just commenting them out.
| assert len(items) == 0 | ||
| assert items == [] |
There was a problem hiding this comment.
[nit] Only one of these conditions is strictly necessary.
| assert len(tai.inventory) == 3 | ||
| assert len(jesse.inventory) == 3 |
There was a problem hiding this comment.
You should not only check the length but also assert that the items you expect to be present in each list are present. For example, you might write:
assert result == True
assert len(tai.inventory) == 3
assert item_a in tai.inventory
assert item_b in tai.inventory
assert item_f in tai.inventory
assert len(jesse.inventory) == 3
assert item_c in jesse.inventory
assert item_d in jesse.inventory
assert item_e in jesse.inventoryYour implementation passes this test. 🙂
There was a problem hiding this comment.
Same comment for the other three tests you filled out below.
| from swap_meet.item import Item | ||
|
|
||
| class Clothing(Item): | ||
| def __init__(self, id = None, fabric = "Unknown", condition=0): |
There was a problem hiding this comment.
[opinion] I prefer space around = like you did with fabric = "Unknown". So condition = 0. You may prefer the other way, but whichever you prefer, I recommend being consistent.
| if item in self.inventory: | ||
| self.inventory.remove(item) | ||
| return item | ||
| else: | ||
| return False |
There was a problem hiding this comment.
[suggestion] If your if body has a return, you technically don't need the else:. You could do:
if item in self.inventory:
self.inventory.remove(item)
return item
return False|
|
||
| def swap_first_item(self, other_vendor): | ||
| if len(self.inventory) == 0 or len(other_vendor.inventory) == 0: | ||
| # if len(self.inventory) or len(other_vendor.inventory) == 0: |
There was a problem hiding this comment.
[nit] You can remove this commented-out line.
| category_list = [] | ||
| for item in self.inventory: | ||
| if item.get_category() == category: | ||
| category_list.append(item) | ||
|
|
||
| return category_list |
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 using a list comprehension as:
result_list = [element for element in source_list if some_condition(element)]Which here would look like:
items_in_category = [item for item in self.inventory if item.get_category() == category]
return items_in_categoryAt first, this may seem more difficult to read, but comprehensions are a very common python style, so I encourage y’all to try working with them!
| of_category = self.get_by_category(category) | ||
| counter = 0 | ||
| if len(of_category) == 0: | ||
| return None | ||
| for item in of_category: | ||
| if item.condition > counter: | ||
| counter = item.condition | ||
| for item in of_category: | ||
| if item.condition == counter: | ||
| return item |
There was a problem hiding this comment.
Nice! Once you're comfortable with the version that uses loops, you can also write this with max:
of_category = self.get_by_category(category)
return max(of_category, key=lambda item: item.condition) if of_category else None| if swapski == True: | ||
| return True | ||
| else: | ||
| return False |
There was a problem hiding this comment.
Whenever you have
if x == True:
return True
else:
return Falseyou can always replace it with
return x
2 participants
[From Matt] I went ahead created a pull request for you so that I could provide you comments in the context of the code.