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

Kate - Lion #20

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

Kate - Lion #20

wants to merge 13 commits into from

Conversation

kate-varinda
Copy link

No description provided.

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.

Excellent job! Your code was good overall. There are a few places where it could have been simpler/cleaner and there was a small logical error in one of the functions. See comments below.

For commits, it would be great if the commit messages could be more descriptive. Instead of writing which wave you completed, it would be good to see something like "Created Item class" or "Added swap_item function".


class Clothing(Item):
def __init__(self, category = "Clothing", condition = 0.0, age = None):

Choose a reason for hiding this comment

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

We actually do not want to have category in the __init__ function. We want the category to always be "Clothing", but if we put category here, the user can set category to whatever they want, which isn't good!

from swap_meet.item import Item

class Decor(Item):

Choose a reason for hiding this comment

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

Great job!

pass
from swap_meet.item import Item

class Electronics(Item):

Choose a reason for hiding this comment

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

Great job!

}

return rating_description[self.condition]

Choose a reason for hiding this comment

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

Excellent! So clean and simple! =D


def condition_description(self):
rating_description = {

Choose a reason for hiding this comment

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

It might be a good idea to have this dictionary outside of the function so that it doesn't have to be recreated every time the function is called.

Comment on lines +117 to +121
assert result == True
assert len(tai.inventory) == 3
assert len(jesse.inventory) == 3
assert all(item in [item_f, item_a, item_b] for item in tai.inventory)
assert all(item in [item_d, item_e, item_c] for item in jesse.inventory)

Choose a reason for hiding this comment

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

Excellent!

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

Choose a reason for hiding this comment

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

It would be better for these that you use a similar assert to what you did for the other tests. == will fail if the two lists are not in the same order but contain the same elements.

Comment on lines +246 to +250
assert result == False
assert len(tai.inventory) == 3
assert len(jesse.inventory) == 3
assert tai.inventory == [item_c, item_b, item_a]
assert jesse.inventory == [item_f, item_e, item_d]

Choose a reason for hiding this comment

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

Great!

from swap_meet.electronics import Electronics

'''
**** TEST ADDED FOR [OPTIONAL] SWAP BY NEWEST FUNCTION ***

Choose a reason for hiding this comment

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

Great job! The tests look great!

return newest_item

def swap_by_newest(self, another_vendor):

Choose a reason for hiding this comment

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

Excellent job taking on the extra work!

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