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

Ocelots - Larissa A. #18

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

Ocelots - Larissa A. #18

wants to merge 10 commits into from

Conversation

ctlaultdel
Copy link

No description provided.

# ------------- WAVE 1 --------------------

def create_movie(title, genre, rating):
pass
def create_movie(title=None, genre=None, rating=None):

Choose a reason for hiding this comment

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

Really excellent use of comments to describe your functions!


@pytest.mark.skip()
# Added assertions to test if correct movie was added to "watched"

Choose a reason for hiding this comment

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

Good addition of the test. And for completeness, we could test the other attributes as well ("genre", "rating")

# create dictionary with movie details
new_movie = {}
keys = ["title", "genre", "rating"]
for key, det in zip(keys, dets):

Choose a reason for hiding this comment

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

Nice use of zip!

for key, det in zip(keys, dets):
new_movie[key] = det
# handle missing movie details
if any(x == None for x in dets):

Choose a reason for hiding this comment

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

nice concise use of any() and this list checking syntax!

Choose a reason for hiding this comment

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

Could this check have occurred earlier in the function, in order to be more efficient and skip unused code?

Choose a reason for hiding this comment

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

Also, good check for None; other cases to validate could include a blank string, ""

# copy user data
updated_data = janes_data.copy()
# iterate through watchlist movies
for movie in updated_data['watchlist']:

Choose a reason for hiding this comment

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

This code passes the tests, but as an aside it contains an "anti-pattern" that is important to be aware of. An element of the list that is being iterated over is being removed inside the for loop. This is generally considered bad practice and can leave you open to problems in more complex code. One solution could be to copy the data you get from "movie in updated_data['watchlist']" into a variable that's not going to change as you modify updated_data.

ratings = []
for movie in janes_data['watched']:
ratings.append(movie['rating'])
if len(ratings) == 0:

Choose a reason for hiding this comment

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

good avoidance of div by zero

if len(genres) == 0:
return None
# collect highest frequency movie genre
popular_genre = statistics.mode(genres)

Choose a reason for hiding this comment

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

Good use of an established specialized library function


# -----------------------------------------
# ------------- WAVE 3 --------------------
# -----------------------------------------


def get_unique_watched(amandas_data):

Choose a reason for hiding this comment

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

Good solution! Can you think of how sets might be used for an even simpler implementation? Though, I do think it's useful to have gone through the foundations and understanding what might be going on behind the scenes with sets.

return amandas_unique_movies


def get_friends_unique_watched(amandas_data):

Choose a reason for hiding this comment

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

Again, sets might help :)

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