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

Jamie McGraner Zoisite C19 #55

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

Conversation

jdanielle12
Copy link

No description provided.

Copy link

@kelsey-steven-ada kelsey-steven-ada left a comment

Choose a reason for hiding this comment

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

Great work y'all! I've left a mix of suggestions and questions to consider for feedback. Please reply here on Github or reach out on Slack if there's anything I can clarify =]

raise Exception("Test needs to be completed.")
# *******************************************************************************************
# ****** Add assertions here to test that the correct movie was added to "watched" **********
assert MOVIE_TITLE_1 in updated_data["watched"][0]["title"]

Choose a reason for hiding this comment

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

Nice assertion for the title! In this case where the test is checking individual keys of a dictionary, I recommend including assertions for all of the relevant keys.

    assert updated_data["watched"][0]["title”] == MOVIE_TITLE_1
    assert updated_data["watched"][0]["rating"] == RATING_1
    assert updated_data["watched"][0]["genre"] == GENRE_1

Comment on lines +184 to +185
assert HORROR_1["title"] == updated_data["watched"][1]["title"]
assert HORROR_1 in updated_data["watched"]

Choose a reason for hiding this comment

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

Something to think about for test assertions, checking that the movie we want has moved lists doesn't guarantee that there were no unexpected changes to any other pieces of data. It can be helpful to confirm all movies are where we expect them:

assert movie_to_watch in updated_data["watched"]
assert FANTASY_2 in updated_data["watched"]
assert FANTASY_1 in updated_data["watchlist"]

Comment on lines +57 to +59
assert HORROR_1 in friends_unique_movies
assert INTRIGUE_3 in friends_unique_movies
assert FANTASY_4 in friends_unique_movies

Choose a reason for hiding this comment

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

Great checks to confirm all the movies we expect are present!

if title == item_title:
watchlist_value.remove(item)
watched_value.append(item)
return user_data

Choose a reason for hiding this comment

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

Great choice to return as soon as the swap was made! We get to save unnecessary processing, and avoid bugs that can be caused by removing elements from a list you're iterating over.


for item in watchlist_value:
item_title = item["title"]
if title == item_title:

Choose a reason for hiding this comment

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

Super teeny nitpick: it looks like there's an extra space after the if here.

Comment on lines +70 to +75
user_unique_movies = []
friends_watched = all_movies_friends_watched(user_data)
for movie in user_data["watched"]:
if movie not in friends_watched:
user_unique_movies.append(movie)
return user_unique_movies

Choose a reason for hiding this comment

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

Great approach! Another variation could use the Set data structure – if we used a Set to hold the titles of the movies in our friends' watched lists, we can then make a single loop over the user’s watched list to generate the unique movies list:

friends_watched = set()
for friend in user_data["friends"]:
    for movie in friend["watched"]:
        friends_watched.add(movie["title"])

user_unique_movies = []
for movie in user_data["watched"]:
    if movie["title"] not in friends_watched:
        user_unique_movies.append(movie)

return user_unique_movies

The benefit is that if we were worried about reducing complexity, if movie not in friends_watched: is O(n) if friends_watched is a list, but it's O(1) when friends_watched is a set. But it is a tradeoff, since we might lose the benefit of reusing the helper function.

# -----------------------------------------
# ------------- WAVE 4 --------------------
# -----------------------------------------

def get_available_recs(user_data):
recommended_movies = []
unique_movies = get_friends_unique_watched(user_data)

Choose a reason for hiding this comment

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

Great code reuse!

Comment on lines +104 to +105
most_frequently_watched = get_most_watched_genre(user_data)
unique_movies = get_friends_unique_watched(user_data)

Choose a reason for hiding this comment

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

😄

return []

for movie in unique_movies:
if movie["genre"] in most_frequently_watched:

Choose a reason for hiding this comment

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

I believe get_most_watched_genre(user_data) returns a string rather than a list. If we use the in operator with strings, then we will get a match if the left operand is a substring of the right operand (for example if we had the words "hat and "chat", "hat" in "chat" would evaluate to True). This isn't an issue for the data set we test with, but is something to watch out for if it was important that we only added genres that were an exact match.

for movie in user_data["favorites"]:
if not movie in friends_watched:
recommended_movies.append(movie)
return recommended_movies

Choose a reason for hiding this comment

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

I love how clear and easy to read these wave 5 functions are!

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.

3 participants