Conversation
…ion that can call attributes and update accordingly.
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.
| assert result == False | ||
| assert len(vendor.inventory) == 3 No newline at end of file |
There was a problem hiding this comment.
👍 This is the expected result, and while making sure the length hasn't changed is a good start, we might also include checks to make sure that the items in the vendor's inventory weren't modified.
| # ********************************************************************* | ||
| # ****** Complete Assert Portion of this test ********** | ||
| # ********************************************************************* | ||
| print("result is " , result) |
There was a problem hiding this comment.
👀 Remove debug output before submitting your project.
And consider using the debugger to break in to the test to see what value result is referring to rather than printing it out.
| # ****** Complete Assert Portion of this test ********** | ||
| # ********************************************************************* | ||
| print("result is " , result) | ||
| assert result is False |
There was a problem hiding this comment.
Though rare, apart from test cases, if we do literally want to compare with one of the boolean constants, we usually use == (and is for None). Though technically, == doesn't guarantee that result is 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.
| assert jolie.inventory == [] | ||
| assert len(fatimah.inventory) == 3 | ||
| assert item_a in fatimah.inventory | ||
| assert item_b in fatimah.inventory | ||
| assert item_c in fatimah.inventory | ||
| assert len(jolie.inventory) == 0 No newline at end of file |
There was a problem hiding this comment.
👍 Great job checking both the result and that no changes happened to the inventories.
Note that the final check (len(jolie.inventory) == 0) is unnecesary since the earlier jolie.inventory == [] already ensures that Jolie's inventory is an empty list (therefore, it has a length of 0).
| # ****** Complete Assert Portion of this test ********** | ||
| # ********************************************************************* | ||
| assert items == [] | ||
| assert len(items) == 0 |
There was a problem hiding this comment.
Again, this check is redundant, since if items compares equal to an empty list, it must have length 0.
While in previous checks, we should check that on failure nothing has moved in the inventories, I think it's fine to omit that here, since we don't generally expect get_by_category to modify the inventory. Checking that n othing had changed is more recommended if the function would otherwise modify some collection or value, but due to a failure, the modification should be prevented.
| def get_vintage_description(self): | ||
| item_age = self.age | ||
| if item_age > 0 and item_age < 20: | ||
| return "Not Vintage" | ||
| elif item_age > 20 and item_age < 30: | ||
| return "Almost Vintage" | ||
| elif item_age > 30 and item_age < 100: | ||
| return "It's vintage" | ||
| elif item_age >100: | ||
| return "SUPER DUPER VINTAGE" | ||
|
|
||
| def get_vintage_status(self): | ||
| if self.age > 30: | ||
| return True | ||
| else: | ||
| return False |
There was a problem hiding this comment.
👍 Nice additional methods using your custom age attribute. We should add tests for these just like you have for get_oldest
| if item_age > 0 and item_age < 20: | ||
| return "Not Vintage" | ||
| elif item_age > 20 and item_age < 30: | ||
| return "Almost Vintage" | ||
| elif item_age > 30 and item_age < 100: | ||
| return "It's vintage" | ||
| elif item_age >100: | ||
| return "SUPER DUPER VINTAGE" |
There was a problem hiding this comment.
It's a little bit more difficult than the condition case since there are ranges of ages here, but we could still make this more data-driven (though maybe not better time complexity). How could we set up a data structure that would let us find the approprate string to use without a long if/elif chain?
| if self.age > 30: | ||
| return True | ||
| else: | ||
| return False |
There was a problem hiding this comment.
Again, we should avoid code like
if conditon:
return True
else:
return FalseInstead, we can return the condition result directly
return self.age > 30| def get_oldest(self): | ||
| return self.get_max_item(self.inventory, "age") |
There was a problem hiding this comment.
👍 Nice custom function using your age data.
| from swap_meet.decor import Decor | ||
| from swap_meet.electronics import Electronics | ||
|
|
||
| def test_get_oldest_items(): |
There was a problem hiding this comment.
👍 Glad to see a test for your get_oldest function.
We'd like to see tests for the custom functions in Item, as well as just constructing Items and subtypes with a variety of age settings.
2 participants
No description provided.