Sapphire - Melinda M.

[MelMott]

No description provided.

[mmcknett-ada]

Green 🟢!

[mmcknett-ada]

You can delete these vs. just commenting them out

[mmcknett-ada]

assert True is always going to pass. Did you mean assert result is True?

[mmcknett-ada]

I appreciate how you ordered these; it made it easy for me to check the arrangement and assertions matched my expectations.

[mmcknett-ada]

You could use assert result is False to make sure that an actual False is returned, vs. just something falsey.

[mmcknett-ada]

You can use Python's special variables for this:

Suggested change

	return "Clothing"
	return self.__class__.__name__ # Python sets this to "Clothing"

[mmcknett-ada]

In fact, since you do this in Item, you don't need to put the get_category overriding functions in your derived classes at all!

[mmcknett-ada]

Because you used a guard clause with early return, it's better to remove this second if statement.

Also, it's important to note that the second clause is not the exact opposite of the guard clause (if you used and it would be, see De Morgan's Law.) If you later changed this function, having both if statements increases the likelihood that you could implicitly return None without realizing.

[mmcknett-ada]

Because your if clause returns, the else: isn't strictly necessary. It's also a good idea to capture the True/False that comes back from swap_items. You could write:

    def swap_first_item(self, other_vendor):
        if not other_vendor.inventory or not self.inventory:
            return False

        # Called method swap_items and used indexing to make sure first items are swapped
        return self.swap_items( other_vendor, self.inventory[0], other_vendor.inventory[0])

[mmcknett-ada]

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:

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

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!

[mmcknett-ada]

Nice! Once you're comfortable with the version that uses loops, you can also write this with max:

by_category = self.get_by_category(category)
return max(by_category, key=lambda item: item.condition) if by_category else None

[mmcknett-ada]

Since swap_items can handle one of the items being None, you can write this as simply:

return self.swap_items(other_vendor, my_best_item, their_best_item)

[MelMott]

Hi Matt, Is there a way to chat about the comments on this? Do I just ask you or can I ask a tutor or anyone with office hours? I just wanted to get clarified on concepts to proper use them in the future. Thank you for grading my program. :) Mel

…

On Thu, Apr 13, 2023, 12:06 PM Matt McKnett (Ada) ***@***.***> wrote: ***@***.**** commented on this pull request. Green 🟢! ------------------------------ In tests/integration_tests/test_wave_01_02_03.py <#72 (comment)>: > @@ -2,7 +2,7 @@ from swap_meet.vendor import Vendor from swap_meet.item import Item ***@***.*** ***@***.*** You can delete these vs. just commenting them out ------------------------------ In tests/unit_tests/test_wave_06.py <#72 (comment)>: > @@ -143,7 +152,15 @@ def test_swap_best_by_category_reordered(): their_priority="Decor" ) - raise Exception("Complete this test according to comments below.") + assert True assert True is always going to pass. Did you mean assert result is True? ------------------------------ In tests/unit_tests/test_wave_06.py <#72 (comment)>: > @@ -143,7 +152,15 @@ def test_swap_best_by_category_reordered(): their_priority="Decor" ) - raise Exception("Complete this test according to comments below.") + assert True + assert len(tai.inventory) == 3 + assert len(jesse.inventory) == 3 + assert item_a in tai.inventory + assert item_b in tai.inventory + assert item_f in tai.inventory + assert item_d in jesse.inventory + assert item_e in jesse.inventory + assert item_c in jesse.inventory I appreciate how you ordered these; it made it easy for me to check the arrangement and assertions matched my expectations. ------------------------------ In tests/unit_tests/test_wave_06.py <#72 (comment)>: > @@ -228,7 +245,15 @@ def test_swap_best_by_category_no_match_is_false(): their_priority="Clothing" ) - raise Exception("Complete this test according to comments below.") + assert not result You could use assert result is False to make sure that an actual False is returned, vs. just something falsey. ------------------------------ In swap_meet/clothing.py <#72 (comment)>: > \ No newline at end of file +import uuid +from swap_meet.item import Item + +class Clothing(Item): + def __init__(self, id=None, condition=0, fabric="Unknown"): + super().__init__(id, condition) + self.fabric = fabric + + def get_category(self): + return "Clothing" You can use Python's special variables for this: ⬇️ Suggested change - return "Clothing" + return self.__class__.__name__ # Python sets this to "Clothing" ------------------------------ In swap_meet/clothing.py <#72 (comment)>: > \ No newline at end of file +import uuid +from swap_meet.item import Item + +class Clothing(Item): + def __init__(self, id=None, condition=0, fabric="Unknown"): [opinion] I prefer spaces around = like this: id = None ------------------------------ In swap_meet/decor.py <#72 (comment)>: > \ No newline at end of file +import uuid Since you don't use uuid, you don't need to import it in this file (or any other one besides item.py) ------------------------------ In swap_meet/item.py <#72 (comment)>: > + if id is None: + self.id = uuid.uuid4().int + else: + self.id = id A ternary is also fine here: self.id = id if id else uuid.uuid4().int ------------------------------ In swap_meet/item.py <#72 (comment)>: > + def __init__(self, id=None, condition=0): + self.condition = condition + if id is None: + self.id = uuid.uuid4().int + else: + self.id = id + + + def __str__(self): + return f"An object of type Item with id {self.id}." + + def get_category(self): + return self.__class__.__name__ + + def condition_description(self): + if self.condition == 0: Nothing constrains self.condition to be an integer. How might you write this function to handle a case where self.condition is, say, 2.5? ------------------------------ In swap_meet/vendor.py <#72 (comment)>: > + if my_item not in self.inventory or their_item not in other_vendor.inventory: + return False + + if my_item in self.inventory or their_item in other_vendor.inventory: Because you used a guard clause with early return, it's better to remove this second if statement. Also, it's important to note that the second clause is *not* the exact opposite of the guard clause (if you used and it would be, see De Morgan's Law <https://en.wikipedia.org/wiki/De_Morgan%27s_laws>.) If you later changed this function, having both if statements increases the likelihood that you could implicitly return None without realizing. ------------------------------ In swap_meet/vendor.py <#72 (comment)>: > + def swap_first_item(self, other_vendor): + if not other_vendor.inventory or not self.inventory: + return False + else: + # Called method swap_items and used indexing to make sure first items are swapped + self.swap_items( other_vendor, self.inventory[0], other_vendor.inventory[0]) + return True Because your if clause returns, the else: isn't strictly necessary. It's also a good idea to capture the True/False that comes back from swap_items. You could write: def swap_first_item(self, other_vendor): if not other_vendor.inventory or not self.inventory: return False # Called method swap_items and used indexing to make sure first items are swapped return self.swap_items( other_vendor, self.inventory[0], other_vendor.inventory[0]) ------------------------------ In swap_meet/vendor.py <#72 (comment)>: > + def get_by_category(self,category): + category_list = [] + + for item in self.inventory: + # Called component method to get a category for comparison in conditional + if category == item.get_category(): + category_list.append(item) + return category_list 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: category_list = [item for item in self.inventory if item.get_category() == category]return category_list 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! ------------------------------ In swap_meet/vendor.py <#72 (comment)>: > + def get_best_by_category(self, category): + + best_item = None + for item in self.get_by_category(category): + # Used component attribute for comparisons + if best_item == None or item.condition > best_item.condition: + best_item = item + return best_item Nice! Once you're comfortable with the version that uses loops, you can also write this with max: by_category = self.get_by_category(category)return max(by_category, key=lambda item: item.condition) if by_category else None ------------------------------ In swap_meet/vendor.py <#72 (comment)>: > + if my_best_item and their_best_item: + self.swap_items(other_vendor, my_best_item, their_best_item) + return True + return False Since swap_items can handle one of the items being None, you can write this as simply: return self.swap_items(other_vendor, my_best_item, their_best_item) ------------------------------ In swap_meet/clothing.py <#72 (comment)>: > \ No newline at end of file +import uuid +from swap_meet.item import Item + +class Clothing(Item): + def __init__(self, id=None, condition=0, fabric="Unknown"): + super().__init__(id, condition) + self.fabric = fabric + + def get_category(self): + return "Clothing" In fact, since you do this in Item, you don't need to put the get_category overriding functions in your derived classes at all! ------------------------------ In swap_meet/item.py <#72 (comment)>: > + if id is None: + self.id = uuid.uuid4().int + else: + self.id = id And what you did in Vendor works, too, assuming you don't care to allow an id of 0. self.id = id or uuid.uuid4().int — Reply to this email directly, view it on GitHub <#72 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A6PAGWAOM4SSZUJC5KYYSP3XBBFEFANCNFSM6AAAAAAWWIRNLQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>