Sphinx - Sarah Koch, Sunitha Nela by sarah-koch · Pull Request #10 · Ada-C22/swap-meet

sarah-koch · 2024-10-03T20:35:29Z

No description provided.

Co-authored-by: nelasunitha <nelasunitha@users.noreply.github.com>

…. Passes first integration test (Waves 1-3). Co-authored-by: nelasunitha <nelasunitha@users.noreply.github.com>

…ntegratins tests for wave_1_3

…. Creates Clothing, Decor, and Electronics classes. Adds further functionality to Item class. Refactors Item and Vendor classes. Co-authored-by: nelasunitha <nelasunitha@users.noreply.github.com>

…_5_6

yangashley

Nice job on swap-meet, Sarah and Sunitha!

I can see from the commit history that you made frequent commits, which is great!

Let me know if you have any questions about my comments.

yangashley · 2024-10-07T22:30:57Z

@@ -1,2 +1,11 @@
-class Clothing:
-    pass
+from swap_meet.item import Item


Nice work importing Item since we're using by name as the parent class of Clothing 👍

yangashley · 2024-10-07T22:31:18Z

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


Nice work on here!

On line 4 you correctly set the default value for fabric to "Unknown"

On line 5 you properly call the initializer of the parent, and pass along the values the child wants to use.

yangashley · 2024-10-07T22:33:18Z

+        basic_info = super().__str__()
+        detailed_info = f" It is made from {self.fabric} fabric."
+        return basic_info + detailed_info


Nice job overriding the super class's __str__ method and invoking super() to reuse some of the logic that has already been written which keeps your code DRY.

You could also do a single line return like:

f"{super().__str__()} It is made from {self.fabric} fabric."

yangashley · 2024-10-07T22:34:49Z

+        self.condition = condition
+
+    def get_category(self):
+        return type(self).__name__


Nice job writing this method in the super class and not repeating the logic in the sub classes 👍

You might also see this written as return self.__class__.__name__

yangashley · 2024-10-07T22:44:36Z

+        if self.condition < 0 or self.condition > 5:
+            return "I don't know what to do with this."


Do we need lines 24-25 if we just return the same string on line 31?

yangashley · 2024-10-07T22:47:51Z

+        my_item = self.inventory[0]
+        their_item = other_vendor.inventory[0]
+
+        return self.swap_items(other_vendor, my_item, their_item)


Good work invoking a function you already wrote to keep your code DRY.

yangashley · 2024-10-07T22:51:36Z

    # *********************************************************************
    # ****** Complete Assert Portion of this test **********
    # *********************************************************************
+    assert result is False


Since the general intention of this method is to modify the inventory, we can check that the items that started in the inventory all remain there and also that inventory list is still 3 items long to make this test more robust.

assert len(vendor.inventory) == 3 assert "a" in vendor.inventory assert "b" in vendor.inventory assert "c" in vendor.inventory

yangashley · 2024-10-07T22:53:03Z

    # *********************************************************************
    # ****** Complete Assert Portion of this test **********
    # *********************************************************************
+    assert not result


More than be falsy, swap_items is intended to return an actual boolean. So while we typically write something like

assert not result

in regular python, here we might prefer the more explicit check of

assert result == False

To make this test more robust we should also verify that the items in the unchanged inventory are actually the items we started with (and not some other 3 items)

assert item_a in fatimah.inventory assert item_b in fatimah.inventory assert item_c in fatimah.inventory

You could also confirm that inventory didn't change by checking the length of Jolie and Fatimah's inventory lists.

yangashley · 2024-10-07T22:56:04Z

+    assert tai.inventory == [item_a, item_b, item_f], f"{tai.inventory}, Expected: [item_a, item_b, item_f]"
+    assert jesse.inventory == [item_d, item_e, item_c], f"{jesse.inventory}, Expected: [item_d, item_e, item_c]"


These assertions will fail if the order of items in each list somehow change. The README didn't specify that the order of inventory items needed to be maintained, so we probably don't need to constrain our tests and therefore our functions to these requirements. In fact, if you were to implement the swap that I suggested in a comment using multiple assignment, then these tests could fail because the order of elements would be changed.

While more verbose, you can check that elements are in a list like:

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

Same comment applies to the assertions you wrote in the test below.

yangashley · 2024-10-07T22:59:41Z

+    def __init__(self, inventory=None):
+        self.inventory = [] if inventory is None else inventory
+
+    def add(self, item):
+        self.inventory.append(item)
+        return item
+
+    def remove(self, item_to_remove):
+        if item_to_remove in self.inventory:
+            self.inventory.remove(item_to_remove)
+            return item_to_remove
+        return False
+
+    def get_by_id(self, id_num):
+        for item in self.inventory:
+            if item.id == id_num:
+                return item
+        return None


yangashley · 2024-10-08T16:58:29Z

+        if item_to_remove in self.inventory:
+            self.inventory.remove(item_to_remove)
+            return item_to_remove
+        return False


This works! However, I'd prefer the logic to be flipped and that the guard clause pattern is used here so you check first for invalid input and then return False.

As the method is currently written, it seems like your main logic is to return False, but the main logic that should be emphasized is that the method removes an item and returns it.

nelasunitha and others added 9 commits September 30, 2024 11:33

first commit

3ae3996

First commit - removed extra comment

905f32a

Adds functionality passing Wave 1 tests. Completes Wave 1 tests.

8af0b4d

Co-authored-by: nelasunitha <nelasunitha@users.noreply.github.com>

Adds functionality to program through Wave 2. Passes all Wave 2 tests.

3649003

Adds functionality to program through Wave 3. Passes all Wave 3 tests…

e3141a7

…. Passes first integration test (Waves 1-3). Co-authored-by: nelasunitha <nelasunitha@users.noreply.github.com>

Added functionality for Wave 4 and passed tests for test_wave_4 and i…

73a6320

…ntegratins tests for wave_1_3

Adds functionality to program through Wave 5. Passes all Wave 5 tests…

9917217

…. Creates Clothing, Decor, and Electronics classes. Adds further functionality to Item class. Refactors Item and Vendor classes. Co-authored-by: nelasunitha <nelasunitha@users.noreply.github.com>

Adds wave 6 completed, all tests passed including integration tests_4…

2dd4f89

…_5_6

Refactors item.condition_description and vendor.get_by_category

56a7360

yangashley reviewed Oct 7, 2024

View reviewed changes

yangashley reviewed Oct 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sphinx - Sarah Koch, Sunitha Nela#10

Sphinx - Sarah Koch, Sunitha Nela#10
sarah-koch wants to merge 9 commits into
Ada-C22:mainfrom
nelasunitha:main

sarah-koch commented Oct 3, 2024

Uh oh!

yangashley left a comment

Uh oh!

yangashley Oct 7, 2024

Uh oh!

yangashley Oct 7, 2024

Uh oh!

yangashley Oct 7, 2024

Uh oh!

yangashley Oct 7, 2024 •

edited

Loading

Uh oh!

yangashley Oct 7, 2024

Uh oh!

yangashley Oct 7, 2024

Uh oh!

yangashley Oct 7, 2024

Uh oh!

yangashley Oct 7, 2024

Uh oh!

yangashley Oct 7, 2024

Uh oh!

yangashley Oct 7, 2024

Uh oh!

yangashley Oct 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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

		if self.condition < 0 or self.condition > 5:
		return "I don't know what to do with this."

		assert tai.inventory == [item_a, item_b, item_f], f"{tai.inventory}, Expected: [item_a, item_b, item_f]"
		assert jesse.inventory == [item_d, item_e, item_c], f"{jesse.inventory}, Expected: [item_d, item_e, item_c]"

Conversation

sarah-koch commented Oct 3, 2024

Uh oh!

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

yangashley Oct 7, 2024 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yangashley Oct 7, 2024 •

edited

Loading