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

Submission project_swap_meet by Sorida Lac C17 #88

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

Conversation

soridalac
Copy link

Thank you for taking time to review my codes and give feedbacks :)

Copy link

@jericahuang jericahuang 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, Sorida! The code you wrote passed all the tests and met the learning goals. You had wonderful code style and great comments throughout. Your project is green 🟢 ! Congrats on completing Unit 1!


def condition_description(self):
CONDITIONS = {

Choose a reason for hiding this comment

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

Lovely use of a constants dictionary to log the condition numbers to their descriptions!

return None
# Return the item that are match category and highest condition even there's duplicate items
return max(self.get_by_category(category), key=attrgetter("condition"))

Choose a reason for hiding this comment

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

Fantastic use of max() with key!


# Return swap best items between Vendor and Vendor friend's from inventory
return self.swap_items(other, my_best_item, their_best_item)

Choose a reason for hiding this comment

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

Great use of .swap_items() as a helper function to DRY your code!

def get_by_category(self, category):
# Return a list of item inventory from that category
return [item for item in self.inventory if item.category == category]

Choose a reason for hiding this comment

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

Nice list comprehension!


# Check if Vendor items not in her inventory or Vendor friend's item not in their inventory
if not my_item in self.inventory or not their_item in other.inventory:

Choose a reason for hiding this comment

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

Great check!

@@ -34,7 +34,11 @@ def test_get_no_matching_items_by_category():

items = vendor.get_by_category("electronics")

raise Exception("Complete this test according to comments below.")
#raise Exception("Complete this test according to comments below.")
assert item_a != items

Choose a reason for hiding this comment

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

Close! != is testing for non-equality. We'd want to check that the item_a, item_b, and item_c are not in items since none of their categories are "electronics".


@pytest.mark.skip
assert tai.get_best_by_category("Decor").category == "Decor"
assert jesse.get_best_by_category("Clothing").category == "Clothing"

Choose a reason for hiding this comment

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

👍🏻

# - That all the correct items are in tai and jesse's inventories
assert item_a in tai.inventory
assert item_b in tai.inventory
assert item_c in tai.inventory

Choose a reason for hiding this comment

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

👍🏻

# - That all the correct items are in tai and jesse's inventories
assert item_a in tai.inventory
assert item_b in tai.inventory
assert item_c in tai.inventory

Choose a reason for hiding this comment

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

👍🏻

# - That all the correct items are in tai and jesse's inventories, and that the items that were swapped are not there

@pytest.mark.skip
assert tai.get_best_by_category("Decor").category == "Decor"

Choose a reason for hiding this comment

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

Nice check! It would also be helpful to assert that the items that were swapped are not in their original inventories!

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