Sea Turtles - Esther Annorzie

[estherannorzie]

No description provided.

[kelsey-steven-ada]

🎉 You completed Unit 1! 🎉 Great work on this project, I've left some suggestions for refactoring. Take a look, and let me know if anything needs clarification!

[kelsey-steven-ada]

We want to comment out the @pytest.mark.skip decorator so the test will run, but we should leave the @pytest.mark.integration_test decorator active for all the tests in the project that use it.

The integration_test decorator directs pytest to run these tests after all of our unit tests. We want to do this because unit tests are small and isolated, making them more helpful for debugging issues. Our integration tests are larger, checking if multiple pieces are working together, which can make it harder to tell where an error came from, if one arises.

[kelsey-steven-ada]

Nice checks for all the relevant data!

[kelsey-steven-ada]

For all of the subclasses of Item, we could remove the category parameter from the child's __init__ method since we don't want folks to change that value. We always want a Clothing object to have the category "Clothing", so we can decide to not even let someone pass a different value.

Suggested change

	def __init__(self, category="", condition=0):
	super().__init__("Clothing", condition)
	def __init__(self, condition=0):
	super().__init__("Clothing", condition)

[kelsey-steven-ada]

Nice handling for condition! It looks like the code assumes that self.condition will always be an integer, but what if we allowed decimals like a condition of 3.5? How could we adapt our code to convert the decimal to an int we could use here, or alternately, change the if-statements to support decimals?

Another approach for finding the condition description could be to make a dictionary that maps conditions as keys to descriptions as values. There's a trade off of using more memory creating the map variable, but if memory isn't a concern, for me, it's a little easier to read.

Suggested change

	if not self.condition:
	return "This item has been heavily used."
	elif self.condition == 1:
	return "This item has been moderately used."
	elif self.condition == 2:
	return "This item has been partially used."
	elif self.condition == 3:
	return "This item is in good condition."
	elif self.condition == 4:
	return "This item is in great condition."
	elif self.condition == 5:
	return "This item is brand new!"
	condition_map = {
	0: "This item has been heavily used.",
	1: "This item has been moderately used.",
	2: "This item has been partially used.",
	3: "This item is in good condition.",
	4: "This item is in great condition.",
	5: "This item is brand new!"
	}
	return condition_map[self.condition]

[kelsey-steven-ada]

Nice handling of the None default value! Another way we could write this is with a ternary operator:

Suggested change

	if not inventory:
	inventory = []
	self.inventory = inventory
	self.inventory = inventory if inventory else []

This can be read as "If the parameter inventory is truthy, assign self.inventory to inventory, else set self.inventory to an empty list". The general blueprint for a ternary operator looks like: variable_name = <value_if_condition_true> if <condition_statement> else <value_if_condition_false>.
More info: https://book.pythontips.com/en/latest/ternary_operators.html

[kelsey-steven-ada]

swapped_items sounds like it might hold Item objects. I'd consider variable names like are_items_swapped or did_swap_items to more clearly represent that this is a boolean value.

[kelsey-steven-ada]

Since we don't perform other actions with swapped_items before we return it, we could remove the variable assignment and directly return the value from calling self.swap_items(...):

Suggested change

	swapped_items = self.swap_items(other, their_best_item, my_best_item)
	return swapped_items
	return self.swap_items(other, their_best_item, my_best_item)

[kelsey-steven-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_matches = [item for item in self.inventory if item.category == category]

At first, this may seem more difficult to read, but comprehensions are a very common python style, so try getting used to working with them!

[kelsey-steven-ada]

I like inverting the logic of the condition to be able to treat it like a guard clause, which allows us to outdent the main logic of the function something like the following:

        if item not in self.inventory:
            return False

        self.inventory.remove(item)
        return item

Another approach we could take is to try to remove the item directly, and handle the ValueError that occurs if it's not there, and return False to handle it.

[kelsey-steven-ada]

Nice use of in since the specification doesn't stipulate where the swapped items will end up in the output!