Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cheetahs 18 - Alyssa R. #80

Open
wants to merge 47 commits into
base: master
Choose a base branch
from
Open

Conversation

alyalyaly18
Copy link

No description provided.

Copy link

@kendallatada kendallatada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Alyssa! Your project has been scored as green.

Great job making frequent commits with descriptive commit messages! ✨

There are several methods in your Vendor class where a try-except block could be implemented to improve time complexity. Checking to see if an item is in a list before doing something with it, is an O(n) operation. So, see if it's possible to replace those if statements with a try-except block instead. :)

Really nice job with your project! I can tell that you took the time to refactor and clean up your code. It looks great! You can find my comments in your code. Let me know if you have any questions. 😊

@@ -34,7 +34,11 @@ def test_get_no_matching_items_by_category():

items = vendor.get_by_category("electronics")

raise Exception("Complete this test according to comments below.")
assert len(items) == 0

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good asserts 👍🏾

assert result
assert len(tai.inventory) == 3
assert len(jesse.inventory) == 3
assert all(item in [item_a, item_b, item_f] for item in tai.inventory)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Niiiice! Beautiful usage of the all()function and a generator expression!

Small recommendation - if you used a set instead of a list for item_a, item_b, item_f, you would improve your time complexity for that item in operation.

}
if self.condition in condition_dict:
return condition_dict[self.condition]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job with your Item class! Love that you implemented a dictionary for the condition descriptions and also have some validation to make sure self.condition is a key in the dictionary. 🥳

def __init__(self, inventory=None):
'''A blueprint for different store inventories.'''

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful docstrings! ✨

class Electronics(Item):

def __init__(self, condition=0):
super().__init__("Electronics", condition)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job with wave 5 and using inheritance! Nice work hardcoding the category too 🥳

def get_by_category(self, category):
'''Return a list of items in the inventory with category.'''
category_list = [i for i in self.inventory if i.category == category]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great list comprehension! 👏🏾

Be sure to use descriptive variable names here too though!

def remove(self, item):
'''Remove an item from inventory list.'''
if item not in self.inventory:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach works, but consider how a try-except block could improve your time complexity. 🤔 💭 😌

def get_best_by_category(self, category):
'''Returns item with the best condition in a certain category.'''
items_in_category = self.get_by_category(category)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice usage of an already implemented function 👍🏾

if not items_in_category:
return None
best_item = max(items_in_category, key=lambda item: item.condition)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooo! Great usage of the max() function with a lambda function!! 🤩

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants