Dragonfly - Tatyana A./ Marina A. #9
Conversation
yangashley
left a comment
yangashley
left a comment
There was a problem hiding this comment.
Nice job on viewing-party! Good to see a decent size number of commits in your commit history.
Let me know if you have questions about my comments.
| # Assert | ||
| assert len(updated_data["watchlist"]) == 0 | ||
| assert len(updated_data["watched"]) == 1 | ||
| assert MOVIE_TITLE_1 in updated_data["watched"][0]["title"] |
There was a problem hiding this comment.
👍 This works, but we're only checking that a string in title matches a string in a list. This test could be made more robust by checking for more than the title. Since watch_movie returns an entire dictionary, it would be good to compare that dictionary with what you expect.
You could arrange an expected result that represents the entire user_data dictionary so that we can check that the watchlist is empty after watching the movie and that the watched list has one movie that was watched. The requirements say that the function should remove the movie from watchlist and move it to watched so we want to test that in addition to the presence of a movie title.
That would look like this:
# Arrange
janes_data = {
"watchlist": [{
"title": MOVIE_TITLE_1,
"genre": GENRE_1,
"rating": RATING_1
}],
"watched": []
}
expected_result = {
"watchlist": [],
"watched": [{
"title": MOVIE_TITLE_1,
"genre": GENRE_1,
"rating": RATING_1
}]
}
# rest of your test
# Assert
assert updated_data == expected_result| # Assert | ||
| assert len(updated_data["watchlist"]) == 1 | ||
| assert len(updated_data["watched"]) == 2 | ||
| assert HORROR_1["title"] in updated_data["watched"][1]["title"] |
There was a problem hiding this comment.
Like my comment for the test above, I'd prefer more robust testing here. Since the function under test, watch_movie, does more than return a string that is a movie title, we should check that the updated user data on line 181 matches what we expect.
The arrange section should create an expected_result dictionary that has one movie in the watchlist and two movies in the watched because movie_to_watch was watched and should be moved.
# Arrange
expected_result = {
"watchlist": [FANTASY_1],
"watched": [FANTASY_2, movie_to_watch]
}
# rest of your test
# Assert
assert updated_data == expected_result|
|
||
| # Assert | ||
| assert len(friends_unique_movies) == 3 | ||
| assert INTRIGUE_3 in friends_unique_movies |
There was a problem hiding this comment.
👍 Nice job with this assertion. Notice that line 56 says that friends_unique_movies should have three elements in it. We should also check that the other 2 movies are in friends_unique_movies.
Also, consider adding an assertion to check that the movie that was added to the first friend's watched list on line 50 was not added to Amanda's watched list.
# Assert
assert len(friends_unique_movies) == 3
assert INTRIGUE_3 in friends_unique_movies
assert HORROR_1 in friends_unique_movies
assert FANTASY_4 in friends_unique_movies
assert INTRIGUE_3 not in amandas_data["watched"]| recommendations = get_new_rec_by_genre(sonyas_data) | ||
|
|
||
| # Assert | ||
| assert len(recommendations) == 0 |
| for movie in user_data["watched"]: | ||
| genre_counts[movie["genre"]] = genre_counts.get(movie["genre"], 0) + 1 | ||
|
|
||
| return max(genre_counts, key=genre_counts.get) |
There was a problem hiding this comment.
Great use of a frequency map and max!
If someone asked you how max works here on line 58, how would you explain that to them?
| for friend in user_data["friends"]: | ||
| for friend_movie in friend["watched"]: | ||
| if user_movie["title"] == friend_movie["title"]: | ||
| none_unique_movie = +1 |
There was a problem hiding this comment.
Do we need none_unique_movie ? How could we write the logic in this method without this variable?
We could directly iterate over all friends and their movies to compile a list of all the movies a friend has watched. (You might even want to put this logic in a helper functions because there are several functions that need to get a list of friends' watched movies so the helper function would reduce repetition.)
Then you could check a user's watched movies against the list of friends' watched movies and add those uniquely watched movies to unique_watched_user_movies.
This would remove a little bit of complexity that we don't need by not having none_unique_movie.
# pseudocode for the approach I described above:
# create a list of movies watched by user
# create a list of movies watched by friends
# check each user's movies against the list of movies watched by friends
# if the user's movie is not in the friend's list then append it to `unique_watched_user_movies`
# return `unique_watched_user_movies`| if friend_movie not in unique_watched_friends_movies: | ||
| unique_watched_friends_movies.append(friend_movie) | ||
|
|
||
| return unique_watched_friends_movies |
There was a problem hiding this comment.
Same comment as above, how could we refactor this function so that we don't need to use none_unique_movie?
| # ------------- WAVE 4 -------------------- | ||
| # ----------------------------------------- | ||
| def get_available_recs(user_data): | ||
| unique_watched_friends_movie = get_friends_unique_watched(user_data) |
There was a problem hiding this comment.
👍 Nice job reusing a function you already wrote to keep your codebase DRY.
|
|
||
| def get_rec_from_favorites(user_data): | ||
| unique_watched_user_movie = get_unique_watched(user_data) | ||
| result = [] |
There was a problem hiding this comment.
Prefer a more descriptive variable name to make your code more self-documenting. Maybe something like recommended_favorite_movies or something to that effect.
3 participants
No description provided.