-
Notifications
You must be signed in to change notification settings - Fork 94
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
Maple- Sabrina L. #94
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heck frickin' yeah, Sabrina!! This was very easy to read, very simple and understandable. I left a few suggestions on how to dry up your code, how to take advantage of inheritance, etc., but your logic looks good.
Great job!! Keep it up!
# super().__init__(category = "Clothing") | ||
## super().__init__(condition) | ||
self.category = "Clothing" | ||
self.condition = condition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great! lines 7 and 8 totally work, though we could definitely use the parent class' constructor instead! It already creates category
and condition
attributes for us, so let's look at that:
# super().__init__(category = "Clothing") | |
## super().__init__(condition) | |
self.category = "Clothing" | |
self.condition = condition | |
# super().__init__(category = "Clothing", condition) |
and I like that you hardcoded "Clothing" since of course this child class Clothing will always be category "Clothing." No reason to give the user the opportunity to mess that up haha
|
||
def __init__(self, condition = 0): | ||
self.category = "Decor" | ||
self.condition = condition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above! we could refactor this a little more to use inheritance more explicitly
pass | ||
from swap_meet.item import Item | ||
|
||
class Electronics(Item): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -1,2 +1,18 @@ | |||
class Item: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -1,2 +1,57 @@ | |||
from swap_meet.item import Item |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so here, we don't actually need to import the literal class Item itself. We aren't instantiating an instance of Item (ie, Item(category="antique",3) ) inside the Vendor anywhere.
There are "items" coming in through parameters, but the class "blueprint" itself can't tell that. On line 7 def add(self, item)
the item
parameter is simply a placeholder. It could be called banana
, apple
, and when this method is actually called we could pass an integer, a string, a dictionary, etc. basically, the method itself doesn't know what these parameter values will be.
So unless we are actually creating an instance inside vendor (like our composition examples), we don't need to import it
else: | ||
return False | ||
|
||
def get_by_category(self, category): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
self.add(their_item) | ||
friend.add(my_item) | ||
self.remove(my_item) | ||
friend.remove(their_item) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great job reusing methods you've already made!we can make this even shorter, though:
self.add(their_item) | |
friend.add(my_item) | |
self.remove(my_item) | |
friend.remove(their_item) | |
self.add(friend.remove(their_item)) | |
friend.add(self.remove(my_item)) |
if you recall, the remove
method returns the item that is removed, so we can call the remove
method right inside the add
else: | ||
return False | ||
|
||
def swap_first_item(self, friend): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
return True | ||
return False | ||
|
||
def get_best_by_category(self, category): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
return best | ||
|
||
|
||
def swap_best_by_category(self, other, my_priority, their_priority): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
No description provided.