Conversation
…tched(user_data) function defintions
| movie_dict = {} | ||
|
|
||
| if not title or not genre or not rating: | ||
| return None | ||
| else: | ||
| movie_dict['title'] = title | ||
| movie_dict['genre'] = genre | ||
| movie_dict['rating'] = rating | ||
|
|
||
| return movie_dict |
There was a problem hiding this comment.
Another way of writing this function could be like so:
def create_movie(title, genre, rating):
if not title or not genre or not rating:
return None
return {
"title": title,
"genre": genre,
"rating": rating
}Since all you are doing is creating a dictionary with the arguments your function is given I would think its okay to not use any variables here.
| for movie in user_data["watchlist"]: | ||
| if movie["title"] == title: | ||
| user_data["watchlist"].remove(movie) | ||
| user_data["watched"].append(movie) |
There was a problem hiding this comment.
Didn't we write a function above that does this? 👀
| if movie["title"] == title: | ||
| user_data["watchlist"].remove(movie) | ||
| user_data["watched"].append(movie) | ||
| break |
|
|
||
| for movie in user_data["watchlist"]: | ||
| if movie["title"] == title: | ||
| user_data["watchlist"].remove(movie) |
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)
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.
|
|
||
| average = total_rating / len(user_data["watched"]) | ||
|
|
||
| return average |
| genre_count = {"Fantasy": 0, "Intrigue": 0, "Action": 0} #change method so that other genres can be included | ||
|
|
||
| if not user_data["watched"]: | ||
| return None | ||
|
|
||
| for movie in user_data["watched"]: | ||
| genre = movie["genre"] | ||
| if genre in genre_count: | ||
| genre_count[genre] += 1 | ||
| top_genre = max(genre_count, key=genre_count.get) | ||
|
|
||
| return top_genre |
There was a problem hiding this comment.
So right now, your code is dependent on if the movie has a genre of one the three listed on line 54. Though it passes the tests, as coders we want to think beyond the tests as well (what is a movie had a genre of romance?). How could we implement a version of this that is agnostic to specific genres? (Hint: Hash Maps/Frequency Maps)
| def get_unique_watched(user_data): | ||
|
|
||
| movies_not_watched_by_friends = [] | ||
|
|
||
| for movie_watched_by_user in user_data["watched"]: | ||
| watched_by_any_friend = False | ||
|
|
||
| # checks if the movie is has been watched by a friend | ||
| for friend in user_data["friends"]: | ||
| if movie_watched_by_user in friend["watched"]: | ||
| watched_by_any_friend = True | ||
| break | ||
|
|
||
| # If no friend has watched it, adds it to the final list | ||
| if not watched_by_any_friend: | ||
| movies_not_watched_by_friends.append(movie_watched_by_user) | ||
|
|
||
| return movies_not_watched_by_friends |
There was a problem hiding this comment.
Great job on completing this function! If I could offer a piece a feedback it would be maybe flattening the lists of friends and their lists of watched movies. That way you can just loop over the a master collection of friends' movies instead of looping over the friends and then their list of movies. We could even make the master collection a dict so don't have to loop over that. The implementation could look like this:
def get_unique_watched(user_data):
user_watched = user_data["watched"]
friends = user_data["friends"]
friend_titles = {}
unique_movies = []
for friend in friends:
watched_movies = friend["watched"]
for movie in watched_movies:
title = movie["title"]
friend_titles[title] = True
for movie in user_watched:
title = movie["title"]
if not friend_titles.get(title):
unique_movies.append(movie)
return unique_moviesThere was a problem hiding this comment.
What do you think the time complexity of implementation like this would be?
| if movie not in user_data["watched"] and movie not in movies_watched_by_friends_not_user: | ||
| movies_watched_by_friends_not_user.append(movie) | ||
|
|
||
| return movies_watched_by_friends_not_user |
There was a problem hiding this comment.
The feedback from the function above could be applied here as well.
|
|
||
| # Populate user_watched set with movie titles from user_data | ||
| for movie in user_data.get("watched"): | ||
| if "title" in movie: |
There was a problem hiding this comment.
Is there a reason why we check to see if title in the movie keys?
| # Loop through each friend and their watched movies | ||
| for friend in user_data.get("friends"): | ||
| for movie in friend.get("watched"): | ||
| # Only add the movie if it's hosted on a subscribed platform and not watched by the user | ||
| if movie["host"] in subscriptions and movie["title"] not in user_watched: |
There was a problem hiding this comment.
Looking back at the first requirements of this function, isn't there a function that completes part of them for us? Specifically these two:
Determine a list of recommended movies. A movie should be added to this list if and only if:
- The user has not watched it
- At least one of the user's friends has watched
- ...
This could clear up a lot of the code that we have in this function.
| genre_counter = {} | ||
| for movie in watched_movies: | ||
| genre = movie["genre"] | ||
| if genre not in genre_counter: | ||
| genre_counter[genre] = 0 | ||
| genre_counter[genre] += 1 |
There was a problem hiding this comment.
This is the frequency map concept I was referring to in this comment. Nice work, implementing it here! Though, don't we have a function that does this? 😉
| if movie not in recommendations: | ||
| recommendations.append(movie) | ||
|
|
||
| return recommendations |
There was a problem hiding this comment.
This is how the function could look if we implemented it using functions that we've written before:
Click to reveal Python code
def get_new_rec_by_genre(user_data):
if not user_data["watched"]:
return []
most_watched_genre = get_most_watched_genre(user_data)
temp_recs = get_available_recs(user_data)
recs = []
for movie in temp_recs:
if movie["genre"] == most_watched_genre:
recs.append(movie)
return recs
| if updated_data["watched"][0] != { | ||
| "title": MOVIE_TITLE_1, | ||
| "genre": GENRE_1, | ||
| "rating": RATING_1 | ||
| }: | ||
| raise Exception("The movie was not moved to the watched.") |
There was a problem hiding this comment.
How this be rewritten with using an assert statement so that we follow the testing format we use with Arrange, Act, Assert?
|
Great work, Christelle & Somy! Your code was well organized and a pleasure to look through! In general, the feedback you will find will be mainly about recognizing situations where you could use functions that you've already written to in implementations you write later down the road. Also, I would suggest to use variables a bit more often so that you can increase the readability of your code. Keep up the good coding! 🎉 |
3 participants
No description provided.