Sharks- Huma Hameed C17 #118
Conversation
| print(new_movie) | ||
| if value == None: | ||
| return None | ||
| return new_movie |
There was a problem hiding this comment.
Nice work!
We can use a guard clause at the very beginning of your function. Right now, your function makes a dictionary then check if the values in the dictionary are None.
If we use a guard clause, then we can just check if title/genre/rating are Falsy (empty lists) and return None without even creating a dictionary and checking it. That would look like:
if not title or not genre or not rating:
return NoneAfter this guard clause, you can build the dictionary literal directly in the return. Then you could remove lines 28 - 32.
Also, you can build the dictionary literal directly in the return:
return {
"title": title,
"genre": genre,
"rating": rating
}
| return user_data | ||
|
|
||
| def add_to_watchlist(user_data, movie): | ||
| user_data["watchlist"] = [] |
There was a problem hiding this comment.
This method looks good!
You'll want to remove line 39 since it actually reassigns the value to an empty list. If the user_data["watchlist"] was not an empty list, then you'd erase what was in there. Other than that, lines 40 and 41 are doing what they need to do.
| # for movies["title"] in user_data["watched"] == title: | ||
| for movies in user_data["watchlist"]: | ||
| if movies["title"] == title: | ||
| print(title) |
There was a problem hiding this comment.
Looks good! just remember to remove debugging print statements when you submit code.
| def get_watched_avg_rating(user_data): | ||
| sum = 0 | ||
| length = len(user_data["watched"]) | ||
| if length==0: |
There was a problem hiding this comment.
Nice use of a guard clause here by putting the if statement at the beginning of your function and then immediately returning 0 without needing to run the rest of the logic in your function. You can also write line 59 like if not user_data["watched"] since an empty list is Falsy, which is more pythonic.
| return 0 | ||
| for movie in user_data["watched"]: | ||
| sum += movie["rating"] | ||
| avg = sum/ length |
There was a problem hiding this comment.
For consistent spacing, add a space after sum so it looks like: avg = sum / length
You can see whitespace rules here in the style guide: https://peps.python.org/pep-0008/#whitespace-in-expressions-and-statements
| # if length==0: | ||
| # return None | ||
| if not user_data["watched"]: | ||
| return None |
There was a problem hiding this comment.
This guard clause looks great!! You could even add it before line 67 so you don't create a dictionary you won't use.
| genre_count[genre] = 1 | ||
| else: | ||
| genre_count[genre] += 1 | ||
| sorted_genre = sorted(genre_count.items(), key = lambda kv:kv[1]) |
There was a problem hiding this comment.
Nice! Another way you might see line 80 written is most_freq_genre = max(genre_count, key=genre_count.get)
If you used this second approach, then what happens is the function .get() would be executed on for each key-value pair in genre_count -- get() returns the value and then max returns the highest integer.
| for movie in friend["watched"]: | ||
| if movie not in friends_movie_list: | ||
| friends_movie_list.append(movie) | ||
| return friends_movie_list |
| print(title) | ||
| user_data["watched"].append(movies) | ||
| user_data["watchlist"].remove(movies) | ||
| return user_data |
There was a problem hiding this comment.
Here I might suggest that you actually add a break in your if block.
Note that, while we might only expect there to be one movie with a particular title in the watchlist (might not be a good assumption, but for this simplified data, it might be ok), this implementation is fine.
As soon as we make a change (like removing from watchlist) we want to stop iterating, so we use break. By adding break after line 49, the loop iterates the watchlist, and when the desired movie is found, adds it to the watched list and removes it from the watchlist. If we returned immediately at this point, everything would be fine!
Without the break statement, we might be in a scenario where title actually matches to more than one movie as it loops (without the break) and we'll see some weirdness.
| # print(unique_list == friends_movies - users_movies) | ||
| # return unique_list == friends_movies - users_movies | ||
| # user_unique_list == list(set(friends_movies)-set(users_movies)) | ||
| # return user_unique_list |
There was a problem hiding this comment.
I think I see what you're trying to do here by casting lists into sets and then getting the difference between them.
When you cast something into a set, the elements need to be immutable, however right now what you have on like line 114, you're casting a list of dictionaries into a set. Dictionaries are mutable and cannot be in sets.
If you want to get the difference between sets, you'll need friends_movies and users_movies to be lists of titles that are strings (not the entire movie dictionary). After you get the difference from the 2 sets that have titles in them, then you'll need a for loop to find all the dictionaries with title in the difference and build a new list with dictionaries.
friends_movies = get_friends_watch_list(user_data) # this would need to be a list of strings
users_movies = get_user_watch_list(user_data) # this would need to be a list of strings
diff = set(users_movies) - set(friends_movies)
unique_list = list(diff)
unique_dict = []
for movie in user_data["watched"]:
if movie["title"] in unique_list:
unique_dict.append(movie)
return unique_dictTry debugging this approach if you have time to see it in action
| for movie in users_movies: | ||
| if movie not in friends_movies: | ||
| user_unique_list.append(movie) | ||
| return user_unique_list |
| return friend_unique_list | ||
| # # print(unique_list == friends_movies - users_movies) | ||
| # # return unique_list == friends_movies - users_movies | ||
| # friend_unique_list == list(set(friends_movies)-set(users_movies)) |
There was a problem hiding this comment.
This implementation looks good! Those user helpers are coming in handy!
See my comment above about sets. Also remember that == is checking equality so line 128 is checking that friend_unique_list is equal to what's on the right side. Were you meaning to use the assignment operator = ?
| rec_movies = [] | ||
| friend_unique = get_friends_unique_watched(user_data) | ||
| for movie in friend_unique: | ||
| if movie["host"]==[] or user_data["subscriptions"]==[]: |
There was a problem hiding this comment.
A more pythonic way of writing this would be
if not movie["host"] or not user_data["subscriptions"]Since you're implementing this check, I might suggest that you return an empty list instead of 0. Since this function expects us to return a list, then when we have empty host or subscription values, then it's good to remain consistent and still return a list (an empty one) instead of a different data type (integer 0)
| if not user_data["watched"] or not friend_unique: | ||
| return genre_rec | ||
| for movie in friend_unique: | ||
| if movie["genre"] in genre: |
There was a problem hiding this comment.
Since genre is a string, you should use == instead of in
| rec_movies = [] | ||
| user_unique = get_unique_watched(user_data) | ||
| if not user_data["favorites"] or not user_unique: | ||
| return 0 |
There was a problem hiding this comment.
Same comment as above, I would suggest you return an empty list so that your function is only returning one data type -- list.
|
Huma! Really great work here! It seems like you even finished before the extension! You've earned a 🟢 and hit the learning goals for this project! I've left some comments about style, guard clauses, and ways you can refactor. We can chat about the set implementation in Wave 3 that you had commented out if you'd like but if you have time, I suggest trying it out with my feedback and debugging it so you get a feel for it. Feel free to refactor and improve on this project. Keep up the great work! |
No description provided.