Skip to content

"Octothorpes - Elaine(SiOk) Sohn(she/her)"#20

Open
sioksohn wants to merge 10 commits intoada-ac2:mainfrom
sioksohn:main
Open

"Octothorpes - Elaine(SiOk) Sohn(she/her)"#20
sioksohn wants to merge 10 commits intoada-ac2:mainfrom
sioksohn:main

Conversation

@sioksohn
Copy link
Copy Markdown

@sioksohn sioksohn commented Dec 9, 2022

No description provided.

Copy link
Copy Markdown

@kelsey-steven-ada kelsey-steven-ada left a comment

Choose a reason for hiding this comment

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

Great work Elaine! I've left a mix of suggestions and questions to consider for feedback. Please reply here on Github or reach out on Slack if there's anything I can clarify =]

Comment thread swap_meet/electronics.py

class Electronics(Item):
def __init__(self, id=None, type="Unknown", condition=0):
super().__init__(id, 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.

Nice use of inheritance across the child classes 👍

Comment thread swap_meet/electronics.py
Comment on lines +8 to +9
def get_category(self):
return f'{self.__class__.__name__}'
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It looks like Clothing and Decor use the parent class' get_category method, but Electronics has its own implementation. Can Electronics use the parent class too?

Comment thread swap_meet/clothing.py
self.fabric = fabric

def __str__(self):
return super().__str__() + f'. It is made from {self.fabric} fabric.' 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 we are using an f-string already, another option would be to call the superclass function from a placeholder:

return f"{super().__str__()}. It is made from {self.fabric} fabric."

Comment thread swap_meet/item.py
Comment on lines +5 to +7
if not id:
id = uuid.uuid4().int
self.id = id
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice solution, another option here could use conditional assignment:

# Assigns self.id to the parameter id if id is truthy, 
# otherwise assigns self.id the return value of uuid.uuid4().int
self.id = id if id else uuid.uuid4().int

Comment thread swap_meet/item.py
return f"An object of type {self.get_category()} with id {self.id}"

def condition_description(self):
if self.condition == 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.

Nice handling for condition description! It looks like the code assumes that self.condition will always be an integer, but what if we allowed decimals like a condition of 3.5? How could we adapt our code to convert the decimal to an int we could use here, or alternately, change the if-statements to support decimals?

Comment thread swap_meet/vendor.py
if not self.inventory:
print("No inventory to display.")
elif not category:
for i, item in enumerate(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.

Great use for enumerate! If we don't want to manually add the +1 to i, enumerate can take a second parameter that tells the counter i where to start: enumerate(self.inventory, start=1)

Comment thread swap_meet/vendor.py
Comment on lines +69 to +80
if not self.inventory:
print("No inventory to display.")
elif not category:
for i, item in enumerate(self.inventory):
print(f"{i+1}. {str(item)}")
else:
display_items = self.get_by_category(category)
if not display_items:
print("No inventory to display.")
else:
for i, item in enumerate(display_items):
print(f"{i+1}. {str(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.

This if-tree is pretty nested and we repeat our looping and no inventory code. If we first resolve what list of items we want to iterate over, then error handle if it is empty, we can remove some of the if-elses:

# If `category` is not empty or None, get a list of items with that category, 
# otherwise set `display_items` to `self.inventory`
display_items = self.get_by_category(category) if category else self.inventory
# The line above would be equivalent to
# if category:
#     display_items = self.get_by_category(category)
# else:
#     display_items = self.inventory

# We can now remove code that was repeated when the list of items was empty
if not display_items:
    print("No inventory to display.") 
    return

# If we've gotten here, there must be an inventory to print
for index, item in enumerate(display_items, start=1):
    print(f"{index}. {item}")

Comment thread swap_meet/vendor.py
print("No inventory to display.")
elif not category:
for i, item in enumerate(self.inventory):
print(f"{i+1}. {str(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.

Because we have implemented the __str__ method, this is one of the situations where python will implicitly call it for us, we don't need to wrap item in a call to str:

print(f"{i+1}. {item}")

result = vendor.remove(item)

raise Exception("Complete this test according to comments below.")
assert result is None
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice assert, we definitely want to confirm our return value is None. We generally also want to confirm that there weren't side affects or changes to the input that we didn't intend. What could we check about the vendor's inventory to confirm that its content didn’t change?

result = fatimah.swap_items(jolie, item_b, nobodys_item)

raise Exception("Complete this test according to comments below.")
assert not 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.

The feedback in wave 1 applies here as well, what else would be helpful to assert to ensure there were no unintended side effects?

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