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 C19 Bella A #67

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

Ruby C19 Bella A #67

wants to merge 3 commits into from

Conversation

bellaiam
Copy link

@bellaiam bellaiam commented Apr 7, 2023

No description provided.

Copy link

@mmcknett-ada mmcknett-ada left a comment

Choose a reason for hiding this comment

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

Green 🟢!

assert item_c in jesse.inventory, "Expected item_c to be in jesse's inventory"
assert item_c not in tai.inventory, "Expected item_c to not be in tai's inventory"
assert item_f not in jesse.inventory, "Expected item_f to not be in jesse's inventory"
# additionals assert statements, I really like writing unit tests

Choose a reason for hiding this comment

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

😄


raise Exception("Complete this test according to comments below.")
# Assert
assert result, "Expected swap_best_by_category to return truthy value"

Choose a reason for hiding this comment

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

You could do assert result is True to make sure that an actual True comes out of the function instead of just a truthy value


# @pytest.mark.skip
def test_swap_by_newest_both_have_same_item():
# Test swapping when both vendors have the same newest item

Choose a reason for hiding this comment

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

It's a little surprising that you might end up with the same instance of an item in two vendors' inventory!

@@ -102,8 +102,7 @@ In Wave 3 we will write a method to stringify (convert to a string) an `Item` us

- When we stringify an instance of `Item` using `str()`, it returns `"An object of type Item with id <id value>."`, where `<id value>` is the `id` of the `Item` instance that `str()` was called on.
- For example, if we had an `Item` instance `item_a = Item(id=12345)`, the output of `str(item_a)` should be `"An object of type Item with id 12345."`.
- To accomplish this, you'll want to investigate what calling `str()` on a class instance does and how you can override such a method. This type of overriding is known as "operator overloading", put simply, it means that the same method exhibits different behavior across instances of different classes. A simple example would be something like `+` which for strings means "concatenate" but for numbers, means "add", or for lists, means "combine".

5

Choose a reason for hiding this comment

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

5?

from swap_meet.item import Item
class Clothing(Item):
def __init__(self, fabric="Unknown", id=None, condition = 0, age = 0):

Choose a reason for hiding this comment

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

[opinion] I prefer spaces around = like this: id = None

return self.swap_items(other_vendor, self.inventory[0], other_vendor.inventory[0])
except IndexError:
return False

Choose a reason for hiding this comment

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

Your guard clause on line 56 should prevent you from hitting any IndexErrors. I don't think you need the try/except.

Comment on lines +64 to +71
items_matching_category = []
# Iterate through the inventory to find items with matching category
for item in self.inventory:
if item.get_category() == category:
items_matching_category.append(item)
# Return the list of matching items
return items_matching_category

Choose a reason for hiding this comment

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

Notice this pattern of:

result_list = []
for element in source_list:
    if some_condition(element):
        result_list.append(element)

can be rewritten using a list comprehension as:

result_list = [element for element in source_list if some_condition(element)]

Which here would look like:

items_matching_category = [item for item in self.inventory if item.get_category() == category]
return items_matching_category

At first, this may seem more difficult to read, but comprehensions are a very common python style, so I encourage y’all to try working with them!

def get_best_by_category(self, category):
# Check if there are any items in the inventory with the given category
if not self.get_by_category(category):

Choose a reason for hiding this comment

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

[suggestion] Put the result of get_by_category in a variable to reuse it, since you call it twice in this function.

Comment on lines +87 to +89
# If either of these conditions is not met, return False as the swap cannot happen.
return False

Choose a reason for hiding this comment

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

You can eliminate this guard clause and just return self.swap_items(other_vendor, my_item, their_item) below, since swap_items handles None values.

def swap_by_newest(self, other_vendor):
# Sort the inventory of both vendors by the age of the items
my_inventory_sorted = sorted(self.inventory, key=lambda item: item.age)

Choose a reason for hiding this comment

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

Using min instead of sorted might simplify this for you. Nice job doing the optional enhancement!

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