Skip to content

Ocelots - Jennifer Dai#16

Open
Jewelhae wants to merge 9 commits intoada-ac2:mainfrom
Jewelhae:main
Open

Ocelots - Jennifer Dai#16
Jewelhae wants to merge 9 commits intoada-ac2:mainfrom
Jewelhae:main

Conversation

@Jewelhae
Copy link
Copy Markdown

@Jewelhae Jewelhae commented Dec 9, 2022

No description provided.

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

#inherit get_category function from Item class, return class name
def get_category(self):
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 there was no additional functionality in the child's method, you didn't have to reimplement this here. It should automatically call the parent class's version if there isn't anything defined in the child :)

Comment thread swap_meet/clothing.py
def __str__(self):
return super().__str__() + f" It is made from {self.fabric} fabric."

def condition_description(self):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same comment as above. No need to redefine a child method if it would just use the inherited parent method.

Comment thread swap_meet/decor.py
self.length = length

#inherit get_category function from superclass Item, return class name
def get_category(self):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same comment as above. No need to redefine a child method if it would just use the inherited parent method.

Comment thread swap_meet/decor.py
def __str__(self):
return super().__str__() + f" It takes up a {self.width} by {self.length} sized space."

def condition_description(self):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same comment as above. No need to redefine a child method if it would just use the inherited parent method.

Comment thread swap_meet/electronics.py
self.type = type

#inherit get_category function from Item class, return class name
def get_category(self):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same comment as above. No need to redefine a child method if it would just use the inherited parent method.

Comment thread swap_meet/electronics.py
def __str__(self):
return super().__str__() + f" This is a {self.type} device."

def condition_description(self):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same comment as above. No need to redefine a child method if it would just use the inherited parent method.

Comment thread swap_meet/vendor.py
self.inventory.append(add_item)
return add_item

#remove duplicates from inventory list if any, else return 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.

I don't know that this comment still matches the function behavior. I don't think it removes duplicates. Rather, it removes an item from the inventory if it existed in the inventory :)

Comment thread swap_meet/vendor.py
#swap item between vendors
def swap_items(self,other_vendor,my_item,their_item):
if my_item in self.inventory and their_item in other_vendor.inventory:
self.inventory.remove(my_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.

Good use of vendor remove() and append(), instead of just directly appending to the vendor item collection!

Comment thread swap_meet/vendor.py
def get_best_by_category(self,category):
item_list = self.get_by_category(category)
best_item = None
temp = 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.

for clarity, i would like to see something like cur_best_condition rather than temp, for clarity

Comment thread swap_meet/vendor.py
if item.get_category()==category:
print(str(index) + ". " + item.__str__())
index += 1
if index == 1:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Does this line make sense here? Is there an indentation error?

Comment thread swap_meet/vendor.py
return self.swap_items(other_vendor,my_best_item,their_best_item)

#print out a string that has all the item in the inventory.
def display_inventory(self,category = ""):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I understand what you're doing in this method. However, having the complex if/else statement, which increments the index in different places, isn't so easy for me to read. And I think that maintenance will be more complicated. In this case, I might have three separate blocks of code (like, separate loops), rather than trying to maintain these different possible behaviors within the same enclosing loop.

Comment thread swap_meet/vendor.py
return self.swap_items(other_vendor,item,item_to_swap)
else:
return self.swap_items(other_vendor,item,item_to_swap)
print (f"No similiar item in the {item_to_swap.get_category} category. Please try another 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 line will never execute, as it's after a return in the same block

assert item_clothing2 in valentina.inventory

# display inventory as expected after swaps
#Note: I shift the expected result item 1 and 2.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thank you for adding this comment :)

Copy link
Copy Markdown

@ilana-adadev ilana-adadev left a comment

Choose a reason for hiding this comment

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

Really well done!

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