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

Carmen Vega - C18 Cheetahs #98

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

Conversation

CarmenVega25
Copy link

No description provided.

Copy link

@jbieniosek jbieniosek left a comment

Choose a reason for hiding this comment

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

Good work on this project Carmen! There's some great code in here, and it's very clean and easy to read. Great use of helper methods in the Vendor class!
This project is yellow due to missing inheritance. One of the primary goals of this project was to practice writing classes that use inheritance, and there isn't any in Clothing, Decor or Electronics classes. I also noticed that you were not able to finish the project. I think we should talk about some additional supports at our next 1:1, including possibly some reduced requirements or extended deadlines to see what you might find helpful.

@@ -1,2 +1,10 @@
from swap_meet.item import Item

class Clothing:

Choose a reason for hiding this comment

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

Suggested change
class Clothing:
class Clothing(Item):

Comment on lines 4 to 7
self.category = "Clothing"
self.condition = condition
self.condition_description = 4

Choose a reason for hiding this comment

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

Clothing should be a subclass of Item. If Clothing is a subclass of Item, it would have access to the condition_description function in Item. It could also use the parent class __init__ function to simplify the code here:

Suggested change
def __init__(self, condition = 0):
self.category = "Clothing"
self.condition = condition
self.condition_description = 4
def __init__(self, condition = 0):
super().__init__("Clothing", condition)


def condition_description(self, condition):
for clothes in range(0, 6):

Choose a reason for hiding this comment

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

This loop isn't needed. The clothes variable isn't used inside the loop, and the conditionals will result in returning on the first iteration.

return "One light scratch"
else:
return "Best condition ever"

Choose a reason for hiding this comment

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

Conditions can be a float, but this set of conditionals will only work for whole numbers. If an item has a condition of 3.5, this function will return "Best condition ever". How could you adapt these conditionals to work for floats?

Comment on lines +6 to +8
inventory = []
self.inventory = inventory

Choose a reason for hiding this comment

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

👍

Comment on lines +31 to +34
friend_list.add(my_item)
self.add(their_item)
self.remove(my_item)

Choose a reason for hiding this comment

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

Great use of helper methods here!

Comment on lines +29 to +30
if their_item in friend_list.inventory:

Choose a reason for hiding this comment

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

Chained conditionals like this can be combined with and:

Suggested change
if my_item in self.inventory:
if their_item in friend_list.inventory:
if my_item in self.inventory and their_item in friend_list.inventory:

Comment on lines +42 to +43
if len(friend_list.inventory) !=0:

Choose a reason for hiding this comment

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

Suggested change
if len(self.inventory) !=0:
if len(friend_list.inventory) !=0:
if len(self.inventory) !=0 and len(friend_list.inventory) !=0:

if len(self.inventory) !=0:
if len(friend_list.inventory) !=0:
self.swap_items(friend_list,self.inventory[0], friend_list.inventory[0])

Choose a reason for hiding this comment

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

Great use of a helper method!

@@ -49,7 +49,8 @@ def test_removing_not_found_is_false():

result = vendor.remove(item)

raise Exception("Complete this test according to comments below.")
#raise Exception("Complete this test according to comments below.")
assert result == False

Choose a reason for hiding this comment

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

I suggest adding a check to verify that the inventory wasn't changed by the remove action:

Suggested change
assert result == False
assert result == False
assert len(vendor.inventory) == 3

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