Skip to content

Sharks/C17/Huma Hameed#108

Open
hhameed1 wants to merge 1 commit intoada-c17:masterfrom
hhameed1:master
Open

Sharks/C17/Huma Hameed#108
hhameed1 wants to merge 1 commit intoada-c17:masterfrom
hhameed1:master

Conversation

@hhameed1
Copy link
Copy Markdown

@hhameed1 hhameed1 commented Apr 8, 2022

No description provided.

Copy link
Copy Markdown

@yangashley yangashley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job, Huma!

All required waves are complete and all required tests are passing! Your code is well organized and you used descriptive variable names. Your general approaches are good, and you're off to a good start with OOP.

I called out some places where you can reverse your logic so you implement a guard clause instead of if/else. This will make your python code more idiomatic. Avoiding unnecessary indenting (by using guard clauses), and checking conditions making use of pythons ideas about truthy and falsy can communicate your intent to other python developers very quickly.

I also pointed out where you can use list comprehension and list destructuring if you're up for it.

There was a place that you could leverage the return value from another method to make your method a little more concise.

Well done 🟢 !

Comment thread swap_meet/clothing.py
string_item = "The finest clothing you could wear."
return string_item

def condition_description(self):
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Clothing inherits from Item, an instance of clothing will automatically have access to the parent class methods (like condition_description).

So if you did pants = Clothing(condition=5) then you could call pants.condition_description() even if you had not written lines 11 and 12.

What's happening on line 11 and 12 is that you create a condition_description method in Clothing which overrides the method in the Item class. However your method here isn't specialized to the Clothing class since it just returns the value from the calling the parent class's method. If you needed to do something else in addition to line 12 then you would override the parent class's method and write additional logic. However, since you're not changing the functionality, you can remove 11 and 12 entirely.

The same comment applies to Electronics and Decor too.

Comment thread swap_meet/decor.py
Comment on lines +8 to +9
string_item = "Something to decorate your space."
return string_item
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also directly return the string literal like this:

 def __str__(self):
       return "Something to decorate your space."

Comment thread swap_meet/item.py
@@ -1,2 +1,30 @@

class Item:
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Item class looks good 👍

Comment thread swap_meet/vendor.py
Comment on lines +10 to +12
if inventory == None:
inventory = []
self.inventory = inventory
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Another way you might see this written is with a ternary, as:

my_var = "some value" if some_condition else "other value"

In this situation, that would look like

self.inventory = [] if inventory is None else inventory

Comment thread swap_meet/vendor.py
Comment on lines +28 to +32
if item in self.inventory:
self.inventory.remove(item)
return item
else:
return False
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A more explicit way to return False would be to use a guard clause instead doing it in the else block. If you use a guard clause, then you can also unindent the logic for handling a valid case where there is something to remove (lines 29 and 30).

if item not in self.inventory:
     return False

self.inventory.remove(item)
     return item

Another approach we could take is to try to remove the item directly, and handle the ValueError that occurs if it's not there, and return False to handle it (try/except)

Comment thread swap_meet/vendor.py
It swaps first item from self and other_vendor and swaps those items\
and returns true
"""
cat_item_list = self.get_by_category(category)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job reusing a method you already wrote

Comment thread swap_meet/vendor.py
Comment on lines +84 to +89
for item in cat_item_list:
item_condition = item.condition
if best_condition < item_condition:
best_condition = item_condition
best_item = item
return best_item
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Comment thread swap_meet/vendor.py
Comment on lines +101 to +104
if my_best == None or their_best == None:
return False
self.swap_items(other, my_best, their_best)
return True No newline at end of file
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of how swap_items is implemented, checking if my_best and their_best are valid on line 101 isn't necessary. See line 54 in vendor.py - you have a check in there that makes sure the args passed into swap_items() are valid.

Also swap_items() returns True if swapping happened and False if no swapping happened. Therefore, you can leverage the return value from swap_items() and refactor this method so it looks like this:

def swap_best_by_category(self, other, my_priority, their_priority):
        my_best = self.get_best_by_category(their_priority)
        their_best = other.get_best_by_category(my_priority)
        return self.swap_items(other, my_best, their_best)

# ****** Complete Assert Portion of this test **********
# *********************************************************************

assert result == False No newline at end of file
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Comment on lines +128 to +137
assert len(tai.inventory) == 3
assert item_c not in tai.inventory
assert item_a in tai.inventory
assert item_b in tai.inventory
assert item_f in tai.inventory
assert len(jesse.inventory) == 3
assert item_f not in jesse.inventory
assert item_d in jesse.inventory
assert item_e in jesse.inventory
assert item_c in jesse.inventory
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can combine lines 128 and 133 if you want:
assert len(tai.inventory) == 3 and len(jesse.inventory) == 3

You can use list destructing like this too - a pythonic way of doing it:

assert [item_a, item_b, item_f] == tai.inventory
assert [item_d, item_e, item_c] == jesse.inventory

https://medium.com/@umaramanat66/destructuring-list-in-python-like-javascript-f7d4c0968538

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants