AC2 , Dalia Ali by dnabilali · Pull Request #11 · ada-ac2/swap-meet

dnabilali · 2022-12-09T00:53:18Z

No description provided.

…g the first 3 tests in wave 1

…list

…passing all tests.

… tests

…d repr in clothin class. passing all tests

…ed condition_description method. passing all tests

…wap_best_by_category methods, passed all tests except those for the last method.

…ion in item.py for condition_description

…choose_and_swap_items method in vendor class

…n to float

kelsey-steven-ada

Great work Dalia! I've left a mix of suggestions and questions to consider for feedback. Please reply here on Github or reach out on Slack if there's anything I can clarify =]

kelsey-steven-ada · 2022-12-14T03:11:58Z

+from .item import Item
+
+class Electronics(Item):
+    def __init__(self, id=None, condition =0, type = 'Unknown'):


I'm seeing inconsistent spacing between parameters and the assignment operators to their default values across files. It is common enough inside of function definitions to forgo spaces around the assignment operator and a default value that I recommend choosing either no spaces on either side (id=None) or spaces on both sides (id = None) and being consistent with that usage across all function definitions.

kelsey-steven-ada · 2022-12-14T03:28:35Z

+        self.fabric = fabric
+
+    def __repr__(self):
+        return f"{super().__repr__()}. It is made from {self.fabric} fabric."


Great use of the superclass!

kelsey-steven-ada · 2022-12-14T03:34:41Z

+        super().__init__(id, condition)
+        self.fabric = fabric
+
+    def __repr__(self):


Awesome to see you exploring dunder methods, as you saw both __str__ and __repr__ can meet our test needs. Since the goal of stringifying an object in this project is to create a human-friendly description over a detailed debugging description, I would suggest using __str__ over __repr__.

kelsey-steven-ada · 2022-12-14T05:04:45Z

+    def condition_description(self):
+        if self.condition == 5:
+            description = "like new"
+        elif self.condition >= 4:


I like the use of comparison operators that account for decimal condition values.

kelsey-steven-ada · 2022-12-14T16:33:27Z

+        if type(self.inventory) is not list:
+            raise TypeError("Please enter the inventory as a list.")


I would suggest placing the validation before allocating the instance variable self.inventory. We could make a small change to the if-statement to check if it is not a list or None:

if type(inventory) is not list and inventory is not None: raise TypeError("Please enter the inventory as a list.") # If we get here, the parameter inventory is either a list or is None self.inventory = [] if inventory is None else inventory

kelsey-steven-ada · 2022-12-15T21:50:43Z

+        if not category and self.inventory:
+            result_list = self.inventory
+        elif not self.get_by_category(category) or not self.inventory:
+            print("No inventory to display.")
+            return False
+        else:
+            result_list = self.get_by_category(category)


We could restructure this a little to reduce nesting:

result_list = self.get_by_category(category) if category else self.inventory if not result_list: print("No inventory to display.") return None

kelsey-steven-ada · 2022-12-15T21:51:38Z

+        for i in range(len(result_list)):
+            print(f"{i+1}. {result_list[i]}")
+        return True


Nice use of range to give us an index to work with. Another way we could do this is with the enumerate function:

for index, item in enumerate(result_list, start=1): print(f"{index}. {item}")

kelsey-steven-ada · 2022-12-16T03:50:39Z

+        self.swap_items(other_vendor,my_item, their_item )
+        return True


Since swap_items returns a True/False value we could directly return the result of that:

return self.swap_items(other_vendor, my_item, their_item)

kelsey-steven-ada · 2022-12-16T04:28:33Z

-    # *********************************************************************
-    # ****** Complete Assert Portion of this test **********
-    # *********************************************************************
+    assert result == None


Nice assert, we definitely want to confirm our return value is None. We generally also want to confirm that there weren't side affects or changes to the input that we didn't intend. What could we check about the vendor's inventory to confirm that its content didn’t change?

That makes sense. Then we would check the following:
assert "a" in vendor.inventory
assert "b" in vendor.inventory
assert "c" in vendor.inventory
assert len(vendor.inventory) == 3

I have an additional question, isn't it better to write "assert result is None"? Or I am thinking about "assert not result". Which of the three ways is best?

If we want to know if a variable is pointing to specifically None, we want to use assert result is None. Any comparison directly to None we want to use the is/is not operators with. assert not result is less specific, since the assert will pass if the variable points to a falsy value like 0 or an empty collection as well as None.

kelsey-steven-ada · 2022-12-16T04:31:16Z

+    assert len(fatimah.inventory) == 3
+    assert len(jolie.inventory) == 0
+    assert item_a in fatimah.inventory
+    assert item_b in fatimah.inventory
+    assert item_c in fatimah.inventory
+    assert not result


Great assertions, very complete checks!

dnabilali added 15 commits December 1, 2022 14:17

implemented __init__ and add(item) methods in vendor class and passin…

69c4c31

…g the first 3 tests in wave 1

completed wave 1: added remove(item) method and passed all wave 1 tests

96e0849

validated that inventory in the __init__ of vendor class has to be a …

03ea918

…list

implemented wave 2(item class and get_by_id method in vendor class , …

8c690aa

…passing all tests.

implemented wave 3 (swap_items method in vendor class) and passed all…

f679075

… tests

passed the first integration test

a0dcf9c

implemented wave 4 and 5: swap_first_item in vendor class and init an…

236297b

…d repr in clothin class. passing all tests

completed wave 5: imlemented all 3 children classes for Item, and add…

3692ded

…ed condition_description method. passing all tests

implemented wave 6: added get_by_category, get_best_by_category and s…

cf5ce1d

…wap_best_by_category methods, passed all tests except those for the last method.

completed wave 6: passed all the tests. updated the 0-5 scale definit…

52b7130

…ion in item.py for condition_description

implemented display_inventory method in vendor class, passing all tests

da73e26

implemented swao_by_id method in vendor class, passed all tests

58b4aa5

finished wave 7(passing all unit and integration tests): implemented …

f8ec8f0

…choose_and_swap_items method in vendor class

updated item __init__: changed type of id to int and type of conditio…

da98de1

…n to float

added tests in test file 1&5 to achieve 100% code coverage

1359dd4

kelsey-steven-ada reviewed Dec 16, 2022

View reviewed changes

		if type(self.inventory) is not list:
		raise TypeError("Please enter the inventory as a list.")

		self.swap_items(other_vendor,my_item, their_item )
		return True

Conversation

dnabilali commented Dec 9, 2022

Uh oh!

kelsey-steven-ada left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants