Sharks - Ivana M. #107
Sharks - Ivana M. #107
Conversation
There was a problem hiding this comment.
Really nice job on your first project, Ivana!
You hit the learning goals, your tests pass, and your assert statements in the test look great!
I added comments about making sure your guard clause comes first in a function, renaming some variables to add clarity, and also a comment in Wave 5 about appending to a list inside a for loop. Let me know if you have any questions about my comments.
Feel free to refactor and improve on this project.
You've earned a 🟢 Keep up the great work!
| assert len(updated_data["watchlist"]) == 0 | ||
| assert len(updated_data["watched"]) == 1 | ||
| assert MOVIE_TITLE_1 not in updated_data["watchlist"] | ||
| assert MOVIE_TITLE_1 in updated_data["watched"][0]["title"] |
There was a problem hiding this comment.
Really nice assert statements here.
| pass | ||
| movie = {} | ||
|
|
||
| if title == None or genre == None or rating == None: |
There was a problem hiding this comment.
Nice use of a guard clause here. When we do title == None we are checking if title is equal to None when what we'd like to check is if title is empty.
A pythonic way of doing that is:
if not title or not genre or not rating:
return NoneThis works because an empty list is Falsy.
You can also move your guard clause to be before line 4. We don't need to create an empty dictionary if we're just going to return None when we have empty lists.
| movie["genre"] = genre | ||
| movie["rating"] = rating | ||
|
|
||
| return movie |
There was a problem hiding this comment.
You could also build a dictionary literal directly inside your return statement:
return {
"title": title,
"genre": genre,
"rating": rating
}|
|
||
| def watch_movie(user_data, title): | ||
| movies = user_data["watchlist"] | ||
| for movie in range(len(movies)): |
There was a problem hiding this comment.
Instead of range(len(movies), you could write your for loop like this:
for movie in user_data["watchlist"] then you wouldn't need to use range or len.
| return user_data | ||
|
|
||
| def watch_movie(user_data, title): | ||
| movies = user_data["watchlist"] |
There was a problem hiding this comment.
this is an easy to read statement so if you wanted to access user_data["watchlist"] without saving it into the variable "movies" that's ok too! It's a balance about when to store something into a variable vs just calling the statement, there's not always a right answer.
| if movie not in user_watched_movies: | ||
| if movie not in unique_to_friends: |
There was a problem hiding this comment.
You can also combine these two conditionals into 1 like:
if movie not in user_watched_movies and movie not in unique_to_friends:
unique_to_friends.append(movie)| user_frequent_genre = get_most_watched_genre(user_data) | ||
|
|
||
| recommended_movies = [] | ||
| if user_frequent_genre is not None: |
There was a problem hiding this comment.
Since you're checking that user_frequent_genre is a valid value, you can write if user_frequent_genre: since a string that is not empty is Truthy.
| friends_data = user_data['friends'] | ||
|
|
||
| for friend in friends_data: | ||
| if len(friend['watched']) > 0: |
There was a problem hiding this comment.
Since a list that is not empty is Truthy, you can write if friend['watched']: for this condition
| for friend in friends_data: | ||
| if len(friend['watched']) > 0: | ||
| for movie in friend['watched']: | ||
| if movie not in user_watched_movies and movie['genre'] == user_frequent_genre: |
There was a problem hiding this comment.
Nice use of the and operator here to combine 2 conditions into one if statement.
| if valid_rec: | ||
| recommended_movies.append(movie) |
There was a problem hiding this comment.
I think line 154 and 155 needs to be indented one level deeper so that it is inside (not outside) the for loop iterating over friends_data. Right now, it's not until you finish looping does any appending happen and when the code is like this then some movies don't get properly appended.
| for key, value in genres_dict.items(): | ||
| if counter < value: | ||
| counter += value | ||
| most_frequent_genre = key |
There was a problem hiding this comment.
While we do get the most_frequent_genre returned correctly, in the future you'll want to declare most_frequent_genre outside of this if statement and outside of the for loop. Usually we initiate a variable outside a for loop, then we do comparing inside the for loop and reset the value of the variable based on the comparison. Finally, we'll return the variable.
No description provided.