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

Otters - Nikki Torab #104

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

Otters - Nikki Torab #104

wants to merge 12 commits into from

Conversation

nikkitorab
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.

Fantastic work on this project Nikki! I am a big fan of the unit tests for the age feature. The code is very clean and readable. Great use of helper methods inside of Vendor. This project is green.

Comment on lines +8 to +10
self.condition = condition
self.age = age

Choose a reason for hiding this comment

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

This will work, but it's duplicating code that is already in Item. Using the super constructor allows this function to take advantage of all of the code in Item:

Suggested change
self.category = "Clothing"
self.condition = condition
self.age = age
super().__init__("Clothing", condition, age)


if self.condition == 5:
return "Mint conditon"

Choose a reason for hiding this comment

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

The condition can be a float, so it can contain a decimal. In the cases where the condition has a decimal, this function will return None. There are a few solutions, one is to cast the condition to an int, which will drop the decimal portion of the number and round down. Another possible solution is to test for a range of values (ex: if self.condition <= 5 and self.condition > 4.

Comment on lines +47 to +51
other_vendor.add(my_item)

other_vendor.remove(their_item)
self.add(their_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!

return False

self.swap_items(other_vendor, self.inventory[0], other_vendor.inventory[0])

Choose a reason for hiding this comment

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

💯

highest_rated = item

return highest_rated

Choose a reason for hiding this comment

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

👍

Comment on lines +119 to +130
for item in other.inventory:
if not item.age is None:
their_newest = item
break
if their_newest.age is None:
return False

for item in other.inventory:
if not item.age is None:
if item.age < their_newest.age:
their_newest = item

Choose a reason for hiding this comment

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

This section is the same as lines 106-117. I recommend pulling this out into a helper method.

their_newest = item

self.swap_items(other, my_newest, their_newest)

Choose a reason for hiding this comment

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

👍

# *********************************************************************
# ****** Complete Assert Portion of this test **********
# *********************************************************************
assert item_a not in items
assert item_b not in items
assert item_c not in items

Choose a reason for hiding this comment

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

Tests work better when there is no room for ambiguity. In this case, checking that these items are not in the list of items is great, but it leaves room for there to be some other item that is also not wanted. Adding something like:

    assert len(items) == 0

helps to make it clear that the items list should be empty.

assert item_f in tai.inventory
assert item_c in jesse.inventory
assert item_d in jesse.inventory
assert item_e in jesse.inventory

Choose a reason for hiding this comment

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

💯


# tests for swap_by_newest

def test_swap_newest():

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