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

Panthers_A18 - Lee Reyes and Sika Sarpong #85

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

Conversation

sikasarpong
Copy link

No description provided.

Copy link

@audreyandoy audreyandoy left a comment

Choose a reason for hiding this comment

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

Lee and Sika!

Your submission completed all waves and passed all required tests. Your code was easy to read and utilized a lot of helper functions which made your code very neat!

My main feedback primarily focuses on using the parent constructor and ways to refactor.

Overall, this project is well-deserving of a 🟢 green 🟢 ✨

Keep up the great work 🐈‍⬛ ✨

Comment on lines +3 to +9
def __init__(self, category="Clothing", condition=0, age=0):
super().__init__(category, condition, age)

def __str__(self):
return "The finest clothing you could wear."

Copy link

@audreyandoy audreyandoy Oct 26, 2022

Choose a reason for hiding this comment

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

Setting a default value will still allow us to reassign the value. In this case, every Clothing instance can change its category to 'Electronics':

pants = Clothing('Electronics', 4.0) 
print(pants.category) # returns 'Electronics'
#what are electronic pants? lol

We can move the default assignment of category to the parent super() constructor. This will ensure that every Clothing instance will be assigned 'Clothing' as its category.

pants = Clothing(4.0) 
print(pants.category) # returns clothing

Comment on lines +4 to +9
def __init__(self, category="Decor", condition=0, age=0):
super().__init__(category, condition, age)

def __str__(self):
return "Something to decorate your space."

Choose a reason for hiding this comment

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

Same comment about category in the child class constructor can also be applied here.

Comment on lines +4 to +9
class Electronics(Item):
def __init__(self, category="Electronics", condition=0, age=0):
super().__init__(category, condition, age)

def __str__(self):
return "A gadget full of buttons and secrets."

Choose a reason for hiding this comment

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

Same comment about category in the child class constructor can also be applied here.

Comment on lines +16 to +28
if self.condition == 0:
return "Ew. You don't want that."
elif self.condition == 1:
return "Maybe you need gloves for that."
elif self.condition == 2:
return "Heavily Used"
elif self.condition == 3:
return "Decent"
elif self.condition == 4:
return "Lightly Used"
elif self.condition == 5:
return "Like new!"

Choose a reason for hiding this comment

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

🤣 lol Love the condition descriptions. "maybe you need gloves for that" is my fav.

Comment on lines 0 to 3
class Vendor:
pass No newline at end of file
def __init__(self, inventory=None) -> None:
self.inventory = inventory if inventory is not None else []

Choose a reason for hiding this comment

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

👍 Great work in handling default parameters for mutable types. It's unsafe to set [] as the default value, but we can use None to stand in for the value when not supplied by the caller, and then use a new empty list in its place.

Comment on lines +87 to +91
assert result
assert len(tai.inventory) == 3
assert len(jesse.inventory) == 3
assert item_a and item_b and item_f in tai.inventory
assert item_c and item_d and item_e in jesse.inventory

Choose a reason for hiding this comment

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

👍

Comment on lines +126 to +132
assert result
assert len(tai.inventory) == 3
assert len(jesse.inventory) == 3
assert item_a and item_b and item_f in tai.inventory
assert item_c and item_d and item_e in jesse.inventory
assert item_c not in tai.inventory
assert item_f not in jesse.inventory

Choose a reason for hiding this comment

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

👍

Comment on lines +223 to +231
assert not result
assert len(tai.inventory) == 3
assert len(jesse.inventory) == 3
assert item_a in tai.inventory
assert item_b in tai.inventory
assert item_c in tai.inventory
assert item_d in jesse.inventory
assert item_e in jesse.inventory
assert item_f in jesse.inventory

Choose a reason for hiding this comment

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

👍

Comment on lines +267 to +275
assert not result
assert len(tai.inventory) == 3
assert len(jesse.inventory) == 3
assert item_a in tai.inventory
assert item_b in tai.inventory
assert item_c in tai.inventory
assert item_d in jesse.inventory
assert item_e in jesse.inventory
assert item_f in jesse.inventory

Choose a reason for hiding this comment

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

👍

Comment on lines +286 to +305
def test_swap_by_newest():
# Arrange:
item_a = Decor(age=3)
item_b = Electronics(age=4)
item_c = Decor(age=5)
tai = Vendor(
inventory=[item_a, item_b, item_c]
)

item_d = Clothing(age=2)
item_e = Decor(age=4)
item_f = Clothing(age=4)
jesse = Vendor(
inventory=[item_f, item_e, item_d]
)
# Act
result = tai.swap_by_newest(other=jesse, my_priority="Clothing",
their_priority="Decor")
# Assert
assert result is None

Choose a reason for hiding this comment

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

👍

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.

3 participants