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

Zoisite-Luwam and Jasmin #60

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

Conversation

CodeWithLuwam
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 =]

Comment on lines +163 to +164
assert watch_movie not in updated_data["watched"]
assert watch_movie not in updated_data["watchlist"]

Choose a reason for hiding this comment

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

These asserts aren't doing what you expect. What does the variable watch_movie point to? These lines confirm that watch_movie is not in either the watched or watchlist lists, is that what's intended?

@@ -158,13 +160,15 @@ def test_moves_movie_from_watchlist_to_empty_watched():
# Assert
assert len(updated_data["watchlist"]) == 0
assert len(updated_data["watched"]) == 1

raise Exception("Test needs to be completed.")
assert watch_movie not in updated_data["watched"]

Choose a reason for hiding this comment

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

Best practices are to keep a single space on either side of operators like in and not in

Comment on lines +189 to +190
assert movie_to_watch in updated_data["watched"]
assert movie_to_watch not in updated_data["watchlist"]

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 +67 to +69
assert INTRIGUE_3 in friends_unique_movies
assert HORROR_1 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 for all the relevant movies!

Comment on lines +5 to +9
movies = title, genre, rating
if all(movies):
return {"title": title, "genre": genre, "rating": rating}
else:
return None

Choose a reason for hiding this comment

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

This is a neat use of all! For this solution, we create a tuple, then iterate through it with all. If we were in a low memory environment and had to reduce variables used, we could use the parameter's truthy/falsy values to create a validation check:

if not (title and genre and rating):
    return None

return {"title": title, "genre": genre, "rating": rating}

By flipping the order, we also get to un-nest the intended path of our code.

break
if not already_added:
unique_watched.append(movie)
return unique_watched

Choose a reason for hiding this comment

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

Great approach with nested loops! Another of many possible implementations could use the Set data structure to solve this – 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:

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

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

return unique_watched

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

def get_available_recs(user_data):
recommended_movie =[]
friends_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 +113 to +122
watched_genres = {}
for movie in user_data["watched"]:
genre = movie["genre"]
if genre in watched_genres:
watched_genres[genre] += 1
else:
watched_genres[genre] = 1
if len(watched_genres) == 0:
return []
most_frequent_genre = max(watched_genres, key=watched_genres.get)

Choose a reason for hiding this comment

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

Great use of a frequency map & max! Could we reuse our function get_most_watched_genre to help reduce what we need to write here?

recommended_movies = []
for friend in user_data["friends"]:
for movie in friend["watched"]:
if movie["genre"] == most_frequent_genre and movie not in recommended_movies and movie not in user_data["watched"]:

Choose a reason for hiding this comment

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

Nice conditionals to check what we need! To keep our lines under the best practice of 79 characters, we should split this over a few lines:

if movie["genre"] == most_frequent_genre 
        and movie not in recommended_movies 
        and movie not in user_data["watched"]:
    recommended_movies.append(movie)

for friend in user_data["friends"]:
if movie in friend["watched"]:
has_watched = True
break

Choose a reason for hiding this comment

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

Love the break to exit the loop as soon as we know we don't want to add the movie to the recommendations!

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