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

Ruby - Thea V. #76

Open
wants to merge 28 commits into
base: main
Choose a base branch
from
Open

Ruby - Thea V. #76

wants to merge 28 commits into from

Conversation

chickenoregg
Copy link

No description provided.

…t implementing the super was breaking my wave 5, will come back to this
Copy link

@nancy-harris nancy-harris left a comment

Choose a reason for hiding this comment

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

Great job! There are comments below on places to make the code cleaner/simpler. Since inheritance was not used, this project is yellow. Great job on adding commits often!


import uuid


class Clothing:

Choose a reason for hiding this comment

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

The requirements for the project stated that the Clothing, Decor, and Electronics classes should inherit from the Item class. Since this was not done, this project is a yellow.

Inheritance helps with keeping the code DRY. There is a lot of functionality that is the same between Clothing, Decor, Electronics, and Item that can be eliminated if we use inheritance.

Comment on lines +19 to +20
return f"An object of type {self.get_category()} with id {self.id}. It takes up a {self.width} by {self.length} sized space."

Choose a reason for hiding this comment

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

Instead of creating a helper function that's only one line, this line of code can go into the __str__ method.

import uuid
# from swap_meet import Vendor


class Item:

Choose a reason for hiding this comment

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

Excellent!

#
def __init__(self, inventory=None):

Choose a reason for hiding this comment

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

Excellent!

self.inventory = []

def add(self, item):

Choose a reason for hiding this comment

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

Excellent!

# *********************************************************************
# ****** Complete Assert Portion of this test **********
# *********************************************************************
assert result == False

Choose a reason for hiding this comment

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

Great start! It would be good to also have an assert to make sure the number of items in the inventory hasn't changed and that nothing was accidentally removed.

# *********************************************************************
# ****** Complete Assert Portion of this test **********
# *********************************************************************
assert not result

Choose a reason for hiding this comment

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

Excellent!

# *********************************************************************
# ****** Complete Assert Portion of this test **********
# *********************************************************************
assert len(items) == 0

Choose a reason for hiding this comment

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

Excellent!

Comment on lines +120 to +121
assert tai.inventory == [item_a, item_b, item_f]
assert jesse.inventory == [item_d, item_e, item_c]

Choose a reason for hiding this comment

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

Be careful using == for asserts on lists! This requires that the lists have items in exactly the same order. It would be safer to use in like the other tests in this file.

Comment on lines +156 to +160
result == True
len(tai.inventory) == 3
len(jesse.inventory) == 3
tai.inventory == [item_a, item_b, item_f]
jesse.inventory == [item_d, item_e, item_c]

Choose a reason for hiding this comment

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

It looks like the term assert is missing for these and the other tests in the rest of the file. Please make sure to remember to add assert!

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