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

Rock - Hannah Lumapas #59

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

Rock - Hannah Lumapas #59

wants to merge 15 commits into from

Conversation

hlumapas
Copy link

@hlumapas hlumapas commented Apr 7, 2021

No description provided.

Comment on lines +4 to +26
class Clothing(Item):
'''
Represents a clothing Item
Attributes:
category (str)
condition (int/float)
age (int/float)
'''
def __init__(self, category="", condition=0, age=0):
'''
Inherits methods of the parent class Item
Parameters:
category ("Clothing")
condition (int/float)
age (int/float)
'''
super().__init__("Clothing", condition, age)

def __str__(self):
'''
Stringifies an instance of Clothing
'''
return "The finest clothing you could wear."

Choose a reason for hiding this comment

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

👍 this looks great! let's try and clean up some of these docstring because there are a lot of them and it's harder to read it.

Suggested change
class Clothing(Item):
'''
Represents a clothing Item
Attributes:
category (str)
condition (int/float)
age (int/float)
'''
def __init__(self, category="", condition=0, age=0):
'''
Inherits methods of the parent class Item
Parameters:
category ("Clothing")
condition (int/float)
age (int/float)
'''
super().__init__("Clothing", condition, age)
def __str__(self):
'''
Stringifies an instance of Clothing
'''
return "The finest clothing you could wear."
'''
Represents a clothing Item and Inherits methods of the parent class Item
'''
class Clothing(Item):
def __init__(self, condition=0, age=0):
super().__init__("Clothing", condition, age)
def __str__(self):
return "The finest clothing you could wear."

so, because Clothing class category will always be "clothing," we don't need to give the user the ability to change that in this situation.

also, I've cleaned up the docstring and fir the important bits under one docstring

from swap_meet.item import Item

# ---- Wave 5 ----- #
class Decor(Item):

Choose a reason for hiding this comment

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

👍 as above, think about summarizing your comments into one docstring

condition (int/float)
age (int/float)
'''
def __init__(self, category="", condition=0, age=0):

Choose a reason for hiding this comment

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

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

@@ -0,0 +1,46 @@
# ---- Wave 2 ----- #
class Item:

Choose a reason for hiding this comment

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

👍

from swap_meet.item import Item

# ---- Wave 1 ----- #
class Vendor:

Choose a reason for hiding this comment

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

👍

return swapped

# ---- Wave 4 ----- #
def swap_first_item(self, friend):

Choose a reason for hiding this comment

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

👍 great to see you'rereusingg swap_items

Comment on lines +103 to +111
category_items = self.get_by_category(category)
items_conditions = []
best_item = None
for item in category_items:
items_conditions.append(item.condition)
best_condition = max(items_conditions)
if best_condition == item.condition:
best_item = item
return best_item

Choose a reason for hiding this comment

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

so this works! But there's a lot of for loops going on here. 1 from get_by_category, 1 in the code, and at least 1 in max method under the hood.

Suggested change
category_items = self.get_by_category(category)
items_conditions = []
best_item = None
for item in category_items:
items_conditions.append(item.condition)
best_condition = max(items_conditions)
if best_condition == item.condition:
best_item = item
return best_item
category_items = self.get_by_category(category)
best_item = category_items[0]
for item in category_items:
if item.condition > best_item.condition:
best_item = item
return best_item

best_item = item
return best_item

def swap_best_by_category(self, other, my_priority, their_priority):

Choose a reason for hiding this comment

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

:+1

Comment on lines +135 to +143
category_items = self.get_by_category(category)
items_ages = []
newest_item = None
for item in category_items:
items_ages.append(item.age)
newest_age = max(items_ages)
if newest_age == item.age:
newest_item = item
return newest_item

Choose a reason for hiding this comment

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

we can change this up like we did above on lines 103 - 111

newest_item = item
return newest_item

def swap_newest_by_category(self, other, my_priority, their_priority):

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.

2 participants