Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sharks, Kassidy Buslach Swap Meet #97

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

buslakj
Copy link

@buslakj buslakj commented Apr 8, 2022

No description provided.

Copy link

@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, Kassidy!

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.

There are places where whitespace was inconsistent. I pointed out a couple, not all of them, but linked the Python Style Guide for you. Clean formatting, including whitespaces, helps keep your code neat and easy to read.

For a couple of your methods, I saw that you passed in Vendor so I wrote a little bit about duck typing. Happy to talk about this more if you'd like.

Overall, well done 🟢 !

Comment on lines +8 to +9
return category

Choose a reason for hiding this comment

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

Feel free to directly return the string literal like this:

 def __str__(self):
       return "The finest clothing you could wear."

from pyparsing import condition_as_parse_action

Choose a reason for hiding this comment

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

You can remove unused imports to keep your code clean

@@ -1,2 +1,13 @@
class Electronics:
pass
from sqlite3 import connect

Choose a reason for hiding this comment

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

Remove unused imports


class Clothing(Item):
def __init__(self,category ='', condition = 0):

Choose a reason for hiding this comment

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

White space should be consistent with the Python style guide.

Here we have some missing white spaces. this line can be refactored to look like:

def __init__(self, category = '', condition = 0)

Where there is white space after the comma and white space after the equal sign

You can read more here if you'd like: https://peps.python.org/pep-0008/#whitespace-in-expressions-and-statements


def __str__(self):
item ="Hello World!"

Choose a reason for hiding this comment

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

Whitespace should be added after equal sign for consistency
item = "Hello World!"

Comment on lines +58 to +64
return None
best_condition = categorized_list[0]
for item in categorized_list:
if item.condition > best_condition.condition:
best_condition = item
return best_condition

Choose a reason for hiding this comment

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

Nice! What you have here works!

Suggesting another way you might see this written. A refactor can remove lines 58 and 59. To do so, you'd need to assign best_condition to None instead of the first element in categorized_list.

If best_condition is None and if categorized_list is empty, then nothing happens in the for loop and None is returned on line 64 because best_condition was never reassigned.

If categorized_list is not empty then best_condition gets reassigned to item while looping.

The refactor would look like:

def get_best_by_category(self, category):
        categorized_list = self.get_by_category(category)
      
        best_condition = None
        for item in categorized_list:
            if item.condition > best_condition.condition:
                best_condition = item
        return best_condition 


def swap_best_by_category(self, other, my_priority, their_priority):
vendors_best= other.get_best_by_category(my_priority)

Choose a reason for hiding this comment

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

Add a whitespace to the left of the equal sign for consistency

Comment on lines +70 to +74
return False
else:
self.swap_items(other, my_best, vendors_best)
return True

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 vendors_best are valid here isn't necessary. See line 30 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):
    vendors_best = other.get_best_by_category(my_priority)       
    my_best = self.get_best_by_category(their_priority)
    return self.swap_items(other, my_best, vendors_best)

Comment on lines +123 to +130
assert len(tai.inventory) == 3
assert len(jesse.inventory) == 3
assert item_f in tai.inventory
assert item_a in tai.inventory
assert item_b in tai.inventory
assert item_d in jesse.inventory
assert item_e in jesse.inventory
assert item_c in jesse.inventory

Choose a reason for hiding this comment

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

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

You can use list destructing and write lines 125-130 lines like this too - a pythonic way of doing it:

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

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

return category_list

def swap_items(self, Vendor, my_item, their_item):
Copy link

@yangashley yangashley Apr 11, 2022

Choose a reason for hiding this comment

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

Here you passed in the class Vendor instead of a variable vendor. Maybe this was an accident because of VSCode autocompleting the word?

In Python we don't need to specify types so the Vendor class can just be the variable vendor (with a lowercase v). If you really want to ensure that the parameter actually is a Vendor, you can add an assert isinstance(vendor, Vendor) or if not isinstance(vendor, Vendor): raise TypeError

But that's not recommended in Python.

The point of "duck typing" is that you write a function that takes "anything that acts like a Vendor instance" rather than a function that takes "a Vendor instance".

Here's a resource on duck typing if you're interested in reading more: https://realpython.com/lessons/duck-typing/#:~:text=Duck%20typing%20is%20a%20concept,a%20given%20method%20or%20attribute.

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