Conversation
jbieniosek
left a comment
jbieniosek
left a comment
There was a problem hiding this comment.
Great work on this project June! The code is very clean and easy to read. I do want to flag references and items in memory as an area that might be useful for you to review, and maybe for us to chat about in our next 1:1. This project is green.
| def __init__(self, category = "Clothing", condition = 0.0, age = 0): | ||
| super().__init__( category = "", condition = 0.0, age = 0) | ||
| self.category = category | ||
| self.condition = condition | ||
| self.age = age |
There was a problem hiding this comment.
This setup makes it possible to pass in a value for category that is not "Clothing", which could result in an instance of Clothing that has some other category. Right now the values being passed to the __init__ function of the parent class are default values, so lines 6-8 are necessary to initialize this instance with the values that are being passed in to the constructor. However, changing the values that are being passed into the super constructor makes it possible to use the code in the parent class, and then lines 6-8 will no longer be necessary.
| def __init__(self, category = "Clothing", condition = 0.0, age = 0): | |
| super().__init__( category = "", condition = 0.0, age = 0) | |
| self.category = category | |
| self.condition = condition | |
| self.age = age | |
| def __init__(self, condition = 0.0, age = 0): | |
| super().__init__( category = "Clothing", condition = condition, age = age) |
or
| def __init__(self, category = "Clothing", condition = 0.0, age = 0): | |
| super().__init__( category = "", condition = 0.0, age = 0) | |
| self.category = category | |
| self.condition = condition | |
| self.age = age | |
| def __init__(self, condition = 0.0, age = 0): | |
| super().__init__( "Clothing", condition, age) |
| elif math.floor(self.condition) == 4: | ||
| return "mint" | ||
| elif math.floor(self.condition) == 5: | ||
| return "new" No newline at end of file |
| last_inventory_item = self.inventory[-1] | ||
|
|
||
| return last_inventory_item |
There was a problem hiding this comment.
This last_inventory_item code works, but it's doing some extra work. It is possible to just return item here.
| self.inventory.remove(item) | ||
| return product |
| for item in self_inventory: | ||
| if item == my_item: | ||
| other_vendor.add(item) | ||
| self.remove(item) | ||
|
|
||
| for item in other_inventory: | ||
| if item == their_item: | ||
| self.add(item) | ||
| other_vendor.remove(item) |
There was a problem hiding this comment.
These loops are not necessary here. The check on line 59 has already verified that my_item is in self_inventory and their_item is in other_inventory. The reference to the item in the list will be the same as any other reference to the item, so we do not have to find the item in the list in order to work with it. When we have an object in memory, all of the variables that are pointing to that object have the same value (ie, the reference to that object), and can be used for comparison purposes interchangably. Fantastic use of helper methods, though!
| for item in self_inventory: | |
| if item == my_item: | |
| other_vendor.add(item) | |
| self.remove(item) | |
| for item in other_inventory: | |
| if item == their_item: | |
| self.add(item) | |
| other_vendor.remove(item) | |
| other_vendor.add(my_item) | |
| self.remove(my_item) | |
| self.add(their_item) | |
| other_vendor.remove(their_item) |
| if len(result) == 0: | ||
| return None | ||
|
|
||
| return max(result, key = lambda c: c.condition) |
| my_best_category = self.get_best_by_category(their_priority) | ||
| their_best_category = other.get_best_by_category(my_priority) | ||
|
|
||
| if len(self.inventory) == 0 or len(other.inventory) == 0 or my_best_category is None or their_best_category is None: |
There was a problem hiding this comment.
This conditional only needs to check if the best_category items are not None, because the length of the inventory doesn't tell us anything about the number of items they have in that category.
| if len(self.inventory) == 0 or len(other.inventory) == 0 or my_best_category is None or their_best_category is None: | |
| if my_best_category is None or their_best_category is None: |
| ''' | ||
| # Helpers | ||
| my_best_category = self.get_best_by_category(their_priority) | ||
| their_best_category = other.get_best_by_category(my_priority) |
|
|
||
| raise Exception("Complete this test according to comments below.") | ||
| # raise Exception("Complete this test according to comments below.") | ||
| assert result == False |
There was a problem hiding this comment.
I suggest adding a check to verify that the inventory wasn't changed by the remove action:
| assert result == False | |
| assert result == False | |
| assert len(vendor.inventory) == 3 |
| assert tai.inventory == [item_a, item_b, item_f] | ||
| assert jesse.inventory == [item_d, item_e, item_c] |
There was a problem hiding this comment.
This works, but it requires that the implementation of swap_best_by_category doesn't make changes to the order of the items, and also that the new items are added at the end of the lists. It's longer to write out, but I would suggest using a series of assert item in [person].inventory statements.
2 participants
No description provided.