Phoenix_C22_Beenish_Alejandra by beenishali693 · Pull Request #22 · Ada-C22/swap-meet

beenishali693 · 2024-10-04T04:02:38Z

No description provided.

beenishali693 · 2024-10-04T17:20:29Z

+        my_item_exists = self.get_by_id(my_item.id)
+        thier_item_exists = other_vendor.get_by_id(thier_item.id)


So now I understand that we don't need these two lines and is using up time that we don't need.

Instead in line 66 we could have made our if statement:

if my_item in self.inventory and their_ item in other_vendor.inventory:

We could operate directly on the two inventories, but there's no significant time penalty to your having used the helper function instead. Either approach is O(n).

beenishali693 · 2024-10-04T17:22:40Z

So I just realized that we misspelled all instances of their (we spelled it thier). It doesn't affect the code but it's not correct English! Sorry!

beenishali693 · 2024-10-04T17:39:44Z

+        return f"An object of type {self.get_category()} with id {self.id}."
+
+    def condition_description(self):
+        if self.condition == 5:


for big 0 -- we should have stored the condition and descriptions in a dictionary. Right now it is having to go through and check is self.condition the same as 5, 4, 3, 2, 1, 0 (making it o(n)) but if it was in a dictionary it could just look it up and it would be o(1).
If I am understanding this correctly.

Nice analysis of this. On the one hand, we might say that this is just hardcoded logic and is constant time, but you're exactly right that in the general case, there could be an arbitrary number of conditions, and we would have to write more code to cover those conditions, increasing the number of instructions. In this case, the fact that we had 5 conditions is really an arbitrary limit, and the general case would be to have n conditions.

This same argument can apply to the space complexity. More code is required to handle more cases, leading to linear space complexity (for the general case, even though this specific case looks constant). If we move the data to a dictionary or list, the size of the code would remain constant as we add more conditions, but the size of the data structure would grow.

To get the maximum benefit for the time complexity, we would want to declare the data structure outside the function, so that it only gets initialized a single time, rather than every time the function is called.

While we don't have a test that tries to get the condition description for a floating point condition, we do check that we can make an Item with floating point conditions. What would this logic do if we tried to get the description for an Item having a condition of 3.5?

anselrognlie · 2024-10-06T18:41:02Z

So I just realized that we misspelled all instances of their (we spelled it thier). It doesn't affect the code but it's not correct English! Sorry!

I use the VS Code extension Code Spell Checker to help with this. It understands that snake_case or camelCase has multiple words in a single name, so it even spellchecks parts of names.

anselrognlie

Looks good! Please review my comments, and let me know if you have any questions via Slack, our next 1:1, or office hours. Nice job!

anselrognlie · 2024-10-06T18:25:04Z

  - If there is no matching item in the `inventory`, the method should explicitly return `None`

-### Wave 3
+### Wave 3d


Keep an eye out for unintentional changes to files. Remember to run git status before git add . and undo any changes we might have accidentally made.

anselrognlie · 2024-10-06T18:30:51Z

+        return f"An object of type {self.get_category()} with id {self.id}."
+
+    def condition_description(self):
+        if self.condition == 5:


Nice analysis of this. On the one hand, we might say that this is just hardcoded logic and is constant time, but you're exactly right that in the general case, there could be an arbitrary number of conditions, and we would have to write more code to cover those conditions, increasing the number of instructions. In this case, the fact that we had 5 conditions is really an arbitrary limit, and the general case would be to have n conditions.

This same argument can apply to the space complexity. More code is required to handle more cases, leading to linear space complexity (for the general case, even though this specific case looks constant). If we move the data to a dictionary or list, the size of the code would remain constant as we add more conditions, but the size of the data structure would grow.

To get the maximum benefit for the time complexity, we would want to declare the data structure outside the function, so that it only gets initialized a single time, rather than every time the function is called.

anselrognlie · 2024-10-06T18:36:13Z

+        my_item_exists = self.get_by_id(my_item.id)
+        thier_item_exists = other_vendor.get_by_id(thier_item.id)


We could operate directly on the two inventories, but there's no significant time penalty to your having used the helper function instead. Either approach is O(n).

anselrognlie · 2024-10-06T18:45:49Z

Nice to see how you were trying out cases while working, though usually we wouldn't check in a file like this. A file like this would usually reside outside of the package, and we would import modules from within the package. So I'd expect to see this in the project root folder, rather than in the swap_meet folder.

To prevent sample code like this from being checked in, we can add its path to the .gitignore file, which will make this file invisible to git.

anselrognlie · 2024-10-06T18:48:15Z

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


👍 This is the expected result, but we might also include checks to make sure that the vendor's inventory wasn't modified.

anselrognlie · 2024-10-06T20:11:14Z

+        return best_in_category
+        # another implementation 
+        # try:
+        #     return max(items_in_category, key=lambda item: item.condition)


Rather than doing this with expection logic, the max function also accepts a default value to return if the collection is empty, so we could have max return None for us.

anselrognlie · 2024-10-06T20:13:12Z

+    def swap_best_by_category(self, other_vendor, my_priority, their_priority):
+        best_item_my_priority = other_vendor.get_best_by_category(my_priority)
+        best_item_their_priority = self.get_best_by_category(their_priority)
+        if best_item_my_priority and best_item_their_priority:


If our swap function did some of the safety checks internally, we might not even need this check here, since None wouldn't be found as a value in either list.

anselrognlie · 2024-10-06T20:14:12Z

+        if best_item_my_priority and best_item_their_priority:
+            return self.swap_items_helper(other_vendor, best_item_their_priority, best_item_my_priority)
+        else:
+            return False


If we keep this check, consider rewriting it like this to emphasize the main logic.

if not best_item_my_priority or not best_item_their_priority: return False return self.swap_items_helper(other_vendor, best_item_their_priority, best_item_my_priority)

anselrognlie · 2024-10-06T20:15:29Z

+
+    def swap_by_newest(self, other_vendor):
+        try:
+            my_newest_item = min(self.inventory, key=lambda item: item.age)


Consider making a helper get_by_newest to avoid needing to repeat the min expression here.

anselrognlie · 2024-10-06T20:16:28Z

+            return False 
+        return self.swap_items_helper(other_vendor, my_first_item, thier_first_item)
+
+    def swap_by_newest(self, other_vendor):


👍 Nice implementation of your custom swap_by_newest.

beenishali693 and others added 17 commits September 30, 2024 12:00

Wave 1 completed including Wave 1 tests

a7daffa

Wave 2, failing 1 test

c43a018

Fixed Vendor Method get_by_id. All wave 2 tests now passing

6faf185

Code deleted

abd1de2

Merge conflict

58b79ef

Finished Wave 3 and completed tests

751db78

Changed code

a22ddbb

Merge branch 'main' of https://github.com/beenishali693/swap-meet

99fab6d

Wave 4 test passed

f6b3ff9

Wave 5 test passed

bf58006

Wave 6 semi complete

3b2eb09

Fixed best_by_category to pass tests

9bb20d4

Added inhertiance and made sure tests were running fine

e16e6af

Finished all waves and tests

112429a

Implemented Optional swap_by_newest and tests

5603d5f

Added one more test for swap_by_newest

f17e7af

Minor spacing update

c831ba4

beenishali693 commented Oct 4, 2024

View reviewed changes

anselrognlie reviewed Oct 14, 2024

View reviewed changes

		my_item_exists = self.get_by_id(my_item.id)
		thier_item_exists = other_vendor.get_by_id(thier_item.id)

Conversation

beenishali693 commented Oct 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beenishali693 commented Oct 4, 2024

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!

anselrognlie commented Oct 6, 2024

Uh oh!

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants