Conversation
kendallatada
left a comment
kendallatada
left a comment
There was a problem hiding this comment.
Hi Vera! Your submission has been scored as green. Please check your code to see the comments I left. Let me know if you have any questions. Nice work! :)
|
|
||
|
|
||
| def add(self,item): | ||
| '''Instance method add, which |
| def swap_first_item(self, friend): | ||
| '''Instance method implement swaping process first item''' | ||
|
|
||
| if len(self.inventory) == 0 or len(friend.inventory) == 0: |
There was a problem hiding this comment.
small note - it's more Pythonic to check if a list is empty by doing if not list rather than if len(list) == 0
|
|
||
| if len(self.inventory) == 0 or len(friend.inventory) == 0: | ||
| return False | ||
| self.swap_items(friend,self.inventory[0],friend.inventory[0]) |
There was a problem hiding this comment.
Nice usage of self.swap_items() ✨
| condition in a certain category''' | ||
|
|
||
| best_dict_category = {} | ||
| best_dict_category[consider_category] = 0.0 |
There was a problem hiding this comment.
small note - you can create a dictionary and specify its values in the same line. So, line 64 could be best_dict_category = {consider_category: 0.0} and you wouldn't need line 65.
| # consider given represents string of category, add to dictionary | ||
| # matching category is key and value is rate. Setup first element | ||
| # of dictionary like max and matching other rate, if higher add to dict | ||
| for category_from_inv in self.inventory: |
There was a problem hiding this comment.
small note - variable names should describe the data they are holding. category_from_inv is holding items, not their categories. If you were working in a team of developers, this could cause confusion for other people. Something you might find helpful - before I consider any code I write finished, I like to think about how easy it would be for someone else to pick up my code and understand it.
| best_category = category_from_inv | ||
| if count == 0: | ||
| return None | ||
| return best_category |
There was a problem hiding this comment.
Something to consider - Python doesn't create a local scope for loops which is why you can return best_category outside of the loop. However, most other languages don't allow access to variables created inside of loops. Going forward, it could be good to treat loops as having a local scope to build that habit.
| '''Instance method will swap the item with the best condition | ||
| in a certain category''' | ||
|
|
||
| if len(self.inventory) and len(other.inventory) != 0: |
There was a problem hiding this comment.
- Careful with how you are combining conditional statements with
and. The!= 0check isn't being applied tolen(self.inventory). In order to apply it to both lists, this should be written asif len(self.inventory) != 0 and len(other.inventory) != 0:. Python knowslen(self.inventory)evaluates to a truthy value, which is why your version still works. - This line of code can be simplified to be
if self.inventory and other.inventory:because Python knows to evaluate an empty list as False. This way is also considered more Pythonic. - Consider if this conditional statement is even necessary. Input validation is great but sometimes the rest of the code is set up in a way that it isn't necessary to validate the input. Does the
get_best_by_category()method know how to handle empty lists? If so, is it necessary to check that the list isn't empty here?
| pass No newline at end of file | ||
| def __init__(self, category = None, condition = 0.0): | ||
| if category is None: | ||
| category = "" |
There was a problem hiding this comment.
Strings are immutable objects so it's safe to set an empty string as a default value in your parameters :)
| self.condition = condition | ||
|
|
||
|
|
||
| def __repr__(self): |
There was a problem hiding this comment.
Love that you're using the repr() function!
| @pytest.mark.skip | ||
| @pytest.mark.integration_test | ||
| # @pytest.mark.skip | ||
| # @pytest.mark.integration_test |
There was a problem hiding this comment.
In the future, don't worry about commenting out @pytest.mark.integration. This decorator just specifies that these tests are integration tests so they should run after all the unit tests have been executed. :)
2 participants
No description provided.