swap-meet AC2 - Catherine Bandarchuk by CatherineBandarchuk · Pull Request #3 · ada-ac2/swap-meet

CatherineBandarchuk · 2022-12-07T01:15:20Z

No description provided.

…st_wave_2 passed.

…te swap_items method. Test_wave4

… Class. Test_wave5 passed.

… in Vendor class. Test_wave7 is passed.

…r class. Test_wave7 passed.

kelsey-steven-ada

Great work Kate! 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-14T02:52:01Z

+
+
+    def __str__( self ):
+        return f"An object of type {super().get_category()} with id {str(self.id)}. It is made from {str(self.fabric)} fabric."  


The superclass __str__ function returns f"An object of type {self.get_category()} with id {str(self.id)}". We could use this reduce a little bit of repetition:

base_msg = super().__str__() return f"{base_msg}. It is made from {str(self.fabric)} fabric."

kelsey-steven-ada · 2022-12-14T02:57:43Z

+class Decor( Item ):
+
+    def __init__( self, id = None, condition = None, width = None, length = None ):
+        super().__init__(id, condition)


I'm noticing that in some places there are spaces around keywords or arguments in parentheses. It isn't common to add spaces between parentheses and what goes inside them, to be consistent across the code, I recommend removing them in the future.

kelsey-steven-ada · 2022-12-14T03:00:23Z

+        if fabric == None:
+            self.fabric = "Unknown"
+        else:
+            self.fabric = fabric


Another option here could be a python conditional assignment:

# Assigns self.fabric to the parameter fabric if it is truthy, # otherwise assigns self.fabric the value "Unknown" self.fabric = fabric if fabric else "Unknown"

kelsey-steven-ada · 2022-12-14T03:18:36Z

+        super().__init__( id, condition )
+
+        if fabric == None:
+            self.fabric = "Unknown"


Strings are an immutable type; unlike lists and dictionaries, we could set the default in the function definition to "Unknown", which could let us remove the if-check.

kelsey-steven-ada · 2022-12-14T03:24:38Z

+
+    def __str__(self):
+
+        return f'''An object of type {super().get_category()} with id {str(self.id)}. It takes up a {str(self.width)} by {str(self.lenght)} sized space.'''


We would want to break this up over a couple lines to meet the best practice of keeping lines under 79 characters. One option could be to close the quotes and use a \ at the end of the line:

return f"An object of type {super().get_category()} with id {str(self.id)}. " \ f"It takes up a {str(self.width)} by {str(self.lenght)} sized space."

kelsey-steven-ada · 2022-12-16T03:51:31Z

+        if not my_item or not their_item:
+            return False
+
+        else:


We could remove the else here and unindent return self.swap_items(other_vendor, my_item, their_item) without changing the result of the function.

kelsey-steven-ada · 2022-12-16T04:06:58Z

+        if self.swap_by_id( other_vendor, my_item_id, their_item_id ):
+            return True
+        return False


We could remove the if/else by directly returning the result of self.swap_by_id.

kelsey-steven-ada · 2022-12-16T04:29:09Z

+    assert item_c in tai.inventory
+
+    assert len(jesse.inventory) == 1
+    assert item_e in jesse.inventory


Love the tests you added!

kelsey-steven-ada · 2022-12-16T04:29:19Z

-    # *********************************************************************
-    # ****** 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?

assert len(vendor.inventory) == 3

Definitely! Since knowing the length doesn't guarantee the individual elements are still what we expect, we might also check the elements of the inventory list.

kelsey-steven-ada · 2022-12-16T04:30:25Z

-    # *********************************************************************
+    assert result == False
+    assert len(jolie.inventory) == 0
+    assert len(fatimah.inventory) == fatimah_len_l


Great assertions, I would also suggest checking the inventory contents, since the length being correct doesn't guarantee that the individual elements are still what we expect.

CatherineBandarchuk added 13 commits December 2, 2022 10:39

Create class Vendor for wave 1 unit tests.

be489d6

Create the Item class and the Vendor class method - get_by_id. The te…

555b3e9

…st_wave_2 passed.

Write __str__ method to convert Item.id to string in Item class. Crea…

0d362c1

…te swap_items method. Test_wave4

Create method swap_first_item in vendor class. Test_wave4 pass.

bbc5ee4

Create classes Electronics, Decor, Clothing using inheretence of Item…

fac82c1

… Class. Test_wave5 passed.

Wrote get_by_category, get_best_by_category and swap_best_by_category…

c995b18

… in Vendor class. Test_wave7 is passed.

Added display_iventory, swap_by_id and choose_and_swap_item for Vendo…

95f1d56

…r class. Test_wave7 passed.

Fixed get_by_category method. integration tests passed.

4ce08b8

wrote swap_by_attributes. create unit tests (passed)

1088c04

refactor vendor.py

60a1a81

refactor item.py

2f69e6b

refactor item, decor, clothing and electronics classes

be84c9f

Add method swap_all_clothes_by_attribute. Wrote test(passed)

385f4e9

kelsey-steven-ada reviewed Dec 16, 2022

View reviewed changes



		def __str__( self ):
		return f"An object of type {super().get_category()} with id {str(self.id)}. It is made from {str(self.fabric)} fabric."


		def __str__(self):

		return f'''An object of type {super().get_category()} with id {str(self.id)}. It takes up a {str(self.width)} by {str(self.lenght)} sized space.''' No newline at end of file

Conversation

CatherineBandarchuk commented Dec 7, 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