Skip to content

Masi Nakhjiri-Cheetahs Group#81

Open
masinakh wants to merge 5 commits intoAda-C18:masterfrom
masinakh:master
Open

Masi Nakhjiri-Cheetahs Group#81
masinakh wants to merge 5 commits intoAda-C18:masterfrom
masinakh:master

Conversation

@masinakh
Copy link
Copy Markdown

@masinakh masinakh commented Oct 7, 2022

No description provided.

Copy link
Copy Markdown

@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 Masi! Your project has been scored as green. A few high level notes:

  • Make sure to make frequent commits that have commit messages that describe what code you wrote/changed.

  • Try to make sure your spacing is consistent. I left this as a comment in your code as well but I would highly recommend checking out PEP8 - Python's style guide to get more information on best practices for spacing and whitespace.

  • 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. :)

Nice job overall! You can find my comments in your code. Let me know if you have any questions! 😊

# ****** Complete Assert Portion of this test **********
# *********************************************************************
assert items ==[]
assert len(items) == 0 No newline at end of file
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Since it's impossible for items == [] to be true and len(items) == 0 to be false, you only need one of these assert statements. They're verifying the same thing, just in two different ways.

# - That all the correct items are in tai and jesse's inventories, including the items which were swapped from one vendor to the other
# - That all the correct items are in tai and jesse's inventories,
# including the items which were swapped from one vendor to the other
assert result
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good asserts 👍🏾

Comment thread swap_meet/item.py

class Item:
pass No newline at end of file
def __init__(self,category="",condition =0,age=0):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Small style note - Try to make sure you are being consistent with your spacing.

This line of code should have this spacing (spaces after commas, no spaces around equal signs):

def __init__(self, category="", condition=0, age=0)

You can read more about whitespace best practices in PEP 8 - Python's style guide

Comment thread swap_meet/item.py
return "Hello World!"

def condition_description(self):
for key, value in CONDITION_DESCRIPTION.items():
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The power of dictionaries is that they allow us to access values in O(1) time using a key. So, rather than looping over the CONDITION_DESCRIPTION dictionary and checking to see if each item has self.condition as the key, we can plug self.condition directly into the dictionary as the key, and we'll get back the value associated with it. For example, we could do something like this:

def condition_description(self):
    return CONDITION_DESCRIPTION[self.condition]

It's going to be really important to feel comfortable with using dictionaries and accessing items using a key. So, I would highly recommend reviewing dictionaries and maybe doing some extra practice with them if this still feels challenging.

Comment thread swap_meet/item.py
@@ -1,2 +1,25 @@
CONDITION_DESCRIPTION = {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Small note - I think it's cool that you created a constant for the condition description dictionary, but I would actually recommend keeping this data structure inside your class definition. Because it's outside the class definition, any file that imports this class module can use the constant anyway they would like. For data that should be tied to a class, it's best to have it live inside the class definition so that it can only be accessed the way it was intended to be used.

Comment thread swap_meet/electronics.py
pass
from swap_meet.item import Item

class Electronics(Item):
Copy link
Copy Markdown

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 🥳

Comment thread swap_meet/decor.py
super().__init__(category = "Decor", condition = condition,age=age)


def __str__(self) -> str:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ooo! You're using type hinting here! Very cool! I would recommend being consistent with it. If you use it in one place, you should probably use it in the rest of your code too.

Comment thread swap_meet/vendor.py
return item

def remove(self,item):
if item not in self.inventory:
Copy link
Copy Markdown

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. 🤔 💭 😌

Comment thread swap_meet/vendor.py
if not items:
return None

return max( items, key=lambda item: item.condition )
Copy link
Copy Markdown

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!! 🤩

Comment thread swap_meet/vendor.py

# return highest_item

# #### refactoring the above lines from 49-57 with Lambda
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yayyyy refactoring! 🙌🏾

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