C22 Luqi Xie and Anees Quateja#16
Conversation
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Looks good! Please review my comments, and let me know if you have any questions via Slack, our next 1:1, or office hours. Nice job!
| # ********************************************************************* | ||
| # ****** Complete Assert Portion of this test ********** | ||
| # ********************************************************************* | ||
| assert result == False |
There was a problem hiding this comment.
This line is redundant with line 65.
Of the two, if we do literally want to compare with one of the boolean constants, we usually use == (and is for None). Technically, == wouldn't guarantee that result was literally False (in Python 0 == False and 1 == True), so I could be persuaded to prefer is since we're expecting to literally return either True or False.
Note that in application code, we rarely if ever compare against one of the boolean constants (with if some_val: preferred for a truthy check, and if not some_val: for a falsy one, and bool(some_val) if we just need to evaluate in a boolean context), but in a test that is meant to confirm the specified behavior, it's acceptable.
| # ****** Complete Assert Portion of this test ********** | ||
| # ********************************************************************* | ||
|
|
||
| assert result is False |
There was a problem hiding this comment.
In addition to checking for the expected return value, since swap_items is known to modify the inventories of the vendors, we should also ensure that in this case, the invetories remain unchanged, similar to the test case you implemented in wave 1.
| # ********************************************************************* | ||
| # ****** Complete Assert Portion of this test ********** | ||
| # ********************************************************************* | ||
| assert len(items) == 0 |
There was a problem hiding this comment.
👍 We shouldn't get any matches in this case.
Notice that this checks that items is an empty collection, but not necessarily a list. The function is specified to return an empty list when no items are found, so we might prefer to write this as
assert items == []Or we could add a type()/isinstance() assertion that items is, in fact, a list.
| assert item_a in tai.inventory | ||
| assert item_b in tai.inventory | ||
| assert item_f in tai.inventory | ||
| assert item_d in jesse.inventory | ||
| assert item_e in jesse.inventory | ||
| assert item_c in jesse.inventory |
There was a problem hiding this comment.
👍 Nice check that the items that should move moved, and those that should stay stayed.
| # - That result is truthy | ||
| # - That tai and jesse's inventories are the correct length | ||
| # - That all the correct items are in tai and jesse's inventories, and that the items that were swapped are not there | ||
| assert result == True |
There was a problem hiding this comment.
👍 We need essentially the same checks here as for the previous test.
| if self.condition == 5: | ||
| return "Brand New" | ||
| elif self.condition == 4: | ||
| return "Like New" | ||
| elif self.condition == 3: | ||
| return "Lightly Used" | ||
| elif self.condition == 2: | ||
| return "Moderately Used" | ||
| elif self.condition == 1: | ||
| return "Heavily Used" |
There was a problem hiding this comment.
Writing explicit checks works, but has some drawbacks. In the general case, there could be an arbitrary number of conditions, and we would have to write more code to cover those conditions. What would this code be like if there we 10 condition rankings? 100?
Notice how this is very similar to our snowman drawing code, where initially we picked a string to draw based on some input, but then later, we refactored it by moving those strings into a list, and using the input as an index into the list. So we could think about doing the same thing here.
To get the maximum benefit, we would want to declare the description data structure outside the function (just like snowman did), so that it only gets initialized a single time, rather than every time the function is called.
There was a problem hiding this comment.
While we don't have a test that tries to get the condition description for a floating point condition, we do check that we can make an Item with floating point conditions. What would this logic do if we tried to get the description for an Item having a condition of 3.5?
In the case where we move the description strings into a data structure, how could we provide a useful behavior for values between the integer conditions?
| list_of_objects = [] | ||
| for item in self.inventory: | ||
| if category == item.get_category(): | ||
| list_of_objects.append(item) | ||
| return list_of_objects |
There was a problem hiding this comment.
👍 We need to loop to find all the items with the specified category.
This would be a great opportunity to try using list comprehension syntax.
|
|
||
|
|
||
| def get_best_by_category(self, category): | ||
| if not self.get_by_category(category): |
There was a problem hiding this comment.
👍 Nice reuse of the previously written function.
But rather than calling the function multiple times below, save the result once in a variable, and then use that variable. Otherwise, each time the function is called it will need to iterate through the inventory again.
category_items = self.get_by_category(category) # then use category_items below
if not category_items:
...| best_item = self.get_by_category(category)[0] | ||
| for item in self.get_by_category(category): | ||
| if item.condition > best_item.condition: | ||
| best_item = item |
There was a problem hiding this comment.
👍 Nice logic to loop through and find the item with the best condition.
| if my_best_item.get_category() == their_priority and their_best_item.get_category() == my_priority: | ||
| other_vendor.add(my_best_item) | ||
| self.remove(my_best_item) | ||
| self.add(their_best_item) | ||
| other_vendor.remove(their_best_item) | ||
| return True |
There was a problem hiding this comment.
👀 Notice that this condition check is unnecessary. If we found an item for each of the get_best_by_category calls, they must already be of the proper category, or they wouldn't have been returned as the best of the desired category.
Once we have the two items to swap, this is the same as swapping any two other items, so we could reuse our swap_items method here.
3 participants
No description provided.