Conversation
| new_movie = { | ||
| "title": title, | ||
| "genre": genre, | ||
| "rating": rating | ||
| } | ||
| if title is None or genre is None or rating is None: | ||
| return None | ||
| return new_movie |
There was a problem hiding this comment.
So if we look at the directions in the README we see that we need to return None if:
If title is falsy, genre is falsy, or rating is falsy, this function should return None
Right now your function is only returning None if the values of the arguments of your function are None but what happens if you are one of the values is an empty string or 0? How could we change your function to consider these questions?
| def watch_movie(user_data,title): | ||
| for movies_to_watch in user_data["watchlist"]: | ||
| if title == movies_to_watch["title"]: | ||
| user_data["watched"].append(movies_to_watch) |
There was a problem hiding this comment.
Don't we have a function that does this for us? 👀
| for movies_to_watch in user_data["watchlist"]: | ||
| if title == movies_to_watch["title"]: | ||
| user_data["watched"].append(movies_to_watch) | ||
| user_data["watchlist"].remove(movies_to_watch) |
There was a problem hiding this comment.
So now that we have talked about Big O analysis, we know that .remove has a time complexity of O(n) since it needs to look through every element in a list to find the sought after value. Right now, this loop has an outer loop of O(n) and you when you find the movie to remove you have another O(n) operation. How could we circumvent using remove? (Hint: Think about list comprehension or pop method
Also in this implementation, we will only use remove once, but in other filtering problems we might have to do this for multiple elements so it doesn't hurt to get in the habit now.
| sum_rating = 0.0 | ||
| for movie in user_data["watched"]: | ||
| sum_rating += movie["rating"] | ||
| avg_rating = float(sum_rating/len(user_data["watched"])) |
There was a problem hiding this comment.
The / operator in Python will implicitly result in a float, is there another reason why this is wrapped in the float constructor?
| def get_most_watched_genre(user_data): | ||
| if len(user_data["watched"]) == 0: | ||
| return None | ||
| count_genre = {} | ||
| for movie in user_data["watched"]: | ||
| count_genre[movie["genre"]] = count_genre.get(movie["genre"],0) + 1 | ||
|
|
||
| max_count = 0 | ||
| popular_genre = None | ||
| for gerne,count in count_genre.items(): | ||
| if count > max_count: | ||
| popular_genre = gerne | ||
| max_count = count | ||
| return popular_genre |
There was a problem hiding this comment.
Great function! Love everything about it! ⭐️
| for friend in user_data["friends"]: | ||
| for movie in friend["watched"]: | ||
| friends_watched_titles.append(movie["title"]) |
There was a problem hiding this comment.
Love how you are flattening the list here so you can simplify your following loop!
| friends_watched_titles.append(movie["title"]) | ||
|
|
||
| for movie in user_data["watched"]: | ||
| if movie["title"] not in friends_watched_titles: |
There was a problem hiding this comment.
Right now, since friends_watched_titles is a list this line of code has an O(n) time complexity. What other data structure could we opt to use so we could circumvent the linear complexity and get one that is constant for this line?
| if movie["title"] not in user_watched_titles \ | ||
| and movie not in friends_unique_movies: |
| and movie not in recommended_movies_list: | ||
| recommended_movies_list.append(movie) | ||
|
|
||
| return recommended_movies_list |
There was a problem hiding this comment.
Thinking about the requirements of if a movie can be added to the recommend_movies_list:
- Determine a list of recommended movies. A movie should be added to this list if and only if:
- The movie is in the user's "favorites"
- None of the user's friends have watched it
Don't we have a function that takes care of the second requirement? How could we change our function here so that it utilizes that function?
| and movie["genre"] == fav_genre \ | ||
| and movie not in recs): | ||
| recs.append(movie) | ||
| return recs |
There was a problem hiding this comment.
This function could be implemented without user_watched_titles, I encourage you to refactor to see how. It could make your code here a bit more concise.
| if not user_data["watched"]: | ||
| return recs | ||
| if not any(friend["watched"] for friend in user_data["friends"]): | ||
| return recs # Return empty list since there are no watched movies by friends |
There was a problem hiding this comment.
Usually with guard clauses, we want to check to if the data we are about to work with will cause the logic following the guard clause to result in an undefined value/error. Here the loops will still work as intended and return an empty list for the result. Love to see you experimenting with any method!
| def get_favorite_genre(user_data): | ||
| genre_freq= {} | ||
| fav_genre = None | ||
| max_count = 0 | ||
| for movie in user_data["watched"]: | ||
| genre = movie["genre"] | ||
| genre_freq[genre] = genre_freq.get(genre,0) + 1 | ||
| if genre_freq[genre] > max_count: | ||
| max_count = genre_freq[genre] | ||
| fav_genre =genre | ||
| return fav_genre |
There was a problem hiding this comment.
Seems to me that this does the same thing as get_most_watched_genre.
|
|
||
| # Assumed user_watched_list helper function | ||
| def get_user_watched_list(user_data): | ||
| return [movie["title"] for movie in user_data["watched"]] No newline at end of file |
There was a problem hiding this comment.
Love the helper functions! Now that we know about imports, could we move them to a different file and import them? 👀
| def get_rec_from_favorites(user_data): | ||
| recs = [] | ||
| friend_watched = get_friends_watched_list_unique(user_data) | ||
| friends_watched_titles = [] | ||
|
|
||
| for movie in friend_watched: | ||
| friends_watched_titles.append(movie["title"]) | ||
|
|
||
| for movie in user_data["favorites"]: | ||
| if movie["title"] not in friends_watched_titles: | ||
| recs.append(movie) | ||
| return recs |
| # Act | ||
| recommendations = get_new_rec_by_genre(sonyas_data) | ||
|
|
||
| assert len(recommendations) == 0 |
There was a problem hiding this comment.
What if we did assert len(genre_recommendations) == []? This more readily declares that the genre_recommendations should be an empty list. Also for testing, if someone returned, say an empty string, this test would technically pass.
|
|
||
| raise Exception("Test needs to be completed.") | ||
| # raise Exception("Test needs to be completed.") |
There was a problem hiding this comment.
Looks like this test wasn't completed, what could we assert here to make properly test this case?
| assert updated_data["watched"][-1]["title"] == HORROR_1["title"] | ||
| assert updated_data["watched"][-1]["genre"] == HORROR_1["genre"] | ||
| assert updated_data["watched"][-1]["rating"] == HORROR_1["rating"] |
There was a problem hiding this comment.
✅
This could also be condensed to:
assert updated_data["watched"] == HORROR_1
| assert updated_data["watched"][-1]["title"] == MOVIE_TITLE_1 | ||
| assert updated_data["watched"][-1]["genre"] == GENRE_1 | ||
| assert updated_data["watched"][-1]["rating"] == RATING_1 |
There was a problem hiding this comment.
Nice! This could look like this as well:
assert updated_data["watched"][-1] == {
"title": MOVIE_TITLE_1,
"genre": GENRE_1,
"rating": RATING_1
}3 participants
No description provided.