Ada AC2 - Supriya Chavan by smchavan · Pull Request #13 · ada-ac2/swap-meet

smchavan · 2022-12-09T03:36:31Z

No description provided.

kelsey-steven-ada

Great work Supriya! 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-14T04:46:47Z

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


I'm seeing inconsistent spacing between parameters in functions and function calls across the files. When there is time, I suggest reviewing your code for style consistency across files before opening PRs. I often have this as part of my code clean up checklist. As far as spacing suggestions, I reccommend using a space after the comma separating parameters to add a little more visual separation.

def __init__(self, id = None, category = "", condition = 0, fabric = "Unknown"):

Yes I will also make a list of things to check for for refactoring and add this to it. Will make the changes. Thank you.

kelsey-steven-ada · 2022-12-14T04:47:33Z

+        self.type = type
+
+    def __str__(self):
+        item_message = super().__str__()


Really nice use of inheritance across the child classes 👍

kelsey-steven-ada · 2022-12-14T04:49:58Z

+        if id is None :
+            self.id = uuid.uuid4().int
+        else:
+            self.id = id


Nice solution, another option here could use conditional assignment:

# Assigns self.id to the parameter id if id is truthy, # otherwise assigns self.id the return value of uuid.uuid4().int self.id = id if id else uuid.uuid4().int

I was wondering the how would I raise value error if ID is not integer with this change?
In this example we can check it using test cases as well ?
I made the change in my code

Hmm, are you thinking about if the user passes in a non-integer id argument? In that case I'd likely add a guard statement at the top that checks if id is not None and the type of id is not an int - in that case we would raise an error. If we get past that statement then either the id argument was passed and has an int value, or it is None and we can create our default with uuid.

Yes, I want to make sure if the id entered by user is not an integer it should raise an error. Yes the guard statement will be good at the beginning so if the id is not an int we don't need to assign the id to that instance object. Will make the changes. Thank you.

kelsey-steven-ada · 2022-12-14T04:50:38Z

+# Wave 5
+    def condition_description(self):
+        description_of_condition = ["Mint","Heavily Used","Like New","Gently Used","Best Used","New"]
+        return description_of_condition[self.condition]


Nice handling for condition description! 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?

One option to have decimal condition values I can use multiple if statement to assign condition to the closest integer like if 1<self.condition < 2: self.condition = 1
again if it is 3.6 I want to round it to 4 . Thinking lambda function might help in some way to round the condition but stuck on it.
One idea came to my mind is while returning the condition description we can round the self.condition value as in
return description_of_condition[round(self.condition)]
the tests are passing and I edited the tests for decimal value and checked it. It works. Will try to think about lambda function usage.

kelsey-steven-ada · 2022-12-15T03:36:05Z

+        if inventory is not None:
+            self.inventory = inventory
+        else:
+            self.inventory = []//I would like to know which is best practice in these two


This is an unusual instance where I would suggest to explicitly check is not None instead of relying on the truthy or falsy value. Both ways are perfectly fine in most situations, but if we were short on memory, checking for truthiness would ignore if the user passed in an empty list and create a new empty list instance, since the empty list argument would evaluate to false.

Oh Ok,I was confused on this , but now understood. Will make the change. Thank you.

kelsey-steven-ada · 2022-12-16T03:25:40Z

+        if self.inventory == []: 
+            print("No inventory to display.")
+            return None
+
+        if category:
+            inventory_items_by_category = self.get_by_category(category)
+            if inventory_items_by_category:
+                for index in range(len(inventory_items_by_category)):
+                    print(f"{index +1}. {inventory_items_by_category[index]}")
+            else:
+                print("No inventory to display.")
+        else:
+            for index in range(len(self.inventory)):
+                    print(f"{index +1}. {self.inventory[index]}")


This if-tree is pretty nested and we have very similar sections of looping and no inventory code. If we change the order of checks slightly to first resolve what list of items we want to iterate over, then error handle if it is empty, we can remove some of the if-else statements:

# If `category` is not empty or None, get a list of items with that category, # otherwise set `display_items` to `self.inventory` display_items = self.get_by_category(category) if category else self.inventory # The line above would be equivalent to # if category: # display_items = self.get_by_category(category) # else: # display_items = self.inventory # Since we decided our list of items first, we get to combine # the error handling that was separated before if not display_items: print("No inventory to display.") return # If we've gotten here, there must be an inventory to print for index in range(len(self.inventory)): print(f"{index +1}. {self.inventory[index]}")

Yes it saves lot of extra lines of code and it makes sense to get the items to be displayed by category if given or self inventory and then check if it is empty all at once. Thank you. I made the changes and it worked.

kelsey-steven-ada · 2022-12-16T03:26:41Z

+        if category:
+            inventory_items_by_category = self.get_by_category(category)
+            if inventory_items_by_category:
+                for index in range(len(inventory_items_by_category)):


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(inventory_items_by_category, start=1): print(f"{index}. {item}")

Thank you. I keep forgetting to use enumerate. I should change it.

kelsey-steven-ada · 2022-12-16T04:21:13Z

+    assert result == None
+    assert item not in vendor.inventory
+    assert len(vendor.inventory) == 3


Great assertions, I would also suggest checking the inventory contents, since checks like the length being correct do not guarantee that the individual elements are still what we expect.

Added these three assertions
assert "a" in vendor.inventory
assert "b" in vendor.inventory
assert "c" in vendor.inventory
Thank you.

kelsey-steven-ada · 2022-12-16T04:35:27Z

+    assert result == False
+    assert nobodys_item not in fatimah.inventory
+    assert nobodys_item not in jolie.inventory


The feedback in wave 1 applies here as well, what else would be helpful to assert to ensure there were no unintended side effects?

assert fatimah.inventory == [item_a, item_b, item_c]
assert jolie.inventory == []
assert item_a in fatimah.inventory
assert item_b in fatimah.inventory
assert item_c in fatimah.inventory

Added above assertions in the test . Thank you.

kelsey-steven-ada · 2022-12-16T04:41:54Z

+
+    ########### New Test for code covergae #############################
+#@pytest.mark.skip
+def test_choose_and_swap_items_with_other_inventory_category_given(monkeypatch):


smchavan · 2022-12-16T17:00:48Z

Great work Supriya! 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 =]

Thank you for your feedback and time. I will go through each one this weekend and get back to you if I have any questions about them.

smchavan added 7 commits December 5, 2022 09:27

Running with unit tests

2824370

Running with unit and integration test

cea99d8

Running with unit and integration test

7536b12

Refactored a little bit

98916db

Added tests for Code Coverage

88d8009

Added tests for Code Coverage1

b4e7080

Added more assertions for test and comments

56c4469

kelsey-steven-ada reviewed Dec 16, 2022

View reviewed changes

Worked on all the comments after grading

587a9c1

Conversation

smchavan 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!

smchavan Dec 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

smchavan Dec 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

smchavan commented Dec 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

smchavan Dec 16, 2022 •

edited

Loading

smchavan Dec 18, 2022 •

edited

Loading