C22- Phoenix - Astry Brana and Amber Edwards#2
C22- Phoenix - Astry Brana and Amber Edwards#2Msambere wants to merge 12 commits intoAda-C22:mainfrom
Conversation
created get_available_recs()
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Looks good! Please review my comments, and let me know if you have any questions. Slack, our next 1:1, or office hours are preferred (rather than replying to my comments in GitHub) as I don't have email notifications turned on. Nice job!
| # Assert | ||
| assert len(updated_data["watchlist"]) == 0 | ||
| assert len(updated_data["watched"]) == 1 | ||
| assert updated_data["watched"][-1]["title"] == MOVIE_TITLE_1 |
There was a problem hiding this comment.
While it's probably the case that if the title is correct, the rest of the data is as well, we should check all three pieces of data to be sure. We can either do this by checking them individually, or we could make a movie dictionary instance to compare.
| # Assert | ||
| assert len(updated_data["watchlist"]) == 1 | ||
| assert len(updated_data["watched"]) == 2 | ||
| assert updated_data["watched"][-1]["title"] == movie_to_watch["title"] |
There was a problem hiding this comment.
Here again, we can check all of the values of the movie. And while your implementation does put the watched movie at the end of the list, that wasn't explicitly specified. As a result, an in check using a full movie might better match the specification, since that won't care where in the watched list the movie ends up.
It would probably also be good to ensure that the movie left in the watchlist is the movie we expect, and that the other movie that started in the watched is still there.
| assert FANTASY_4 in friends_unique_movies | ||
| assert HORROR_1 in friends_unique_movies | ||
| assert INTRIGUE_3 in friends_unique_movies |
There was a problem hiding this comment.
👍 Great way to check tht the three expected movies are there without worrying about at what position each ended up.
|
|
||
| # Assert | ||
| assert len(friends_unique_movies) == 3 | ||
| #assert desired movies on in list |
There was a problem hiding this comment.
Nit: Put a space after the # in the comment (minor typo in the comment itself?)
# assert desired movies on in list| # Act | ||
| recommendations = get_new_rec_by_genre(sonyas_data) | ||
|
|
||
| # Assert | ||
| assert len(recommendations) == 0 |
There was a problem hiding this comment.
👍 This test needed both the act and the assert.
| recommendations = [] | ||
| for friend in user_data["friends"]: | ||
| for movie in friend["watched"]: | ||
| if movie["host"] in user_data["subscriptions"] and movie not in user_data["watched"] and movie not in recommendations: |
There was a problem hiding this comment.
If we started with the movies only watched by friends (get_friends_unique_watched ), we could avoid the check against the movies the user has watched. The code would also be simpler, since most of the looping is also already done in the helper.
| if not user_data["watched"] or not user_data["friends"]: | ||
| return recommendations |
There was a problem hiding this comment.
Even without these checks, the logic would return the desired [] result if the user hadn't watched any movies, or had no friends that watched movies.
| most_watched_genre = get_most_watched_genre(user_data) | ||
| friend_unique_watched = get_friends_unique_watched(user_data) |
There was a problem hiding this comment.
👍 Great use of functions written for previous waves. All we have left to do is filter the friends movies by the user's preferred genre.
| for movie in friend_unique_watched: | ||
| if movie["genre"] == most_watched_genre: | ||
| recommendations.append(movie) |
There was a problem hiding this comment.
This would be an ideal case to use a list comprehension.
recommendations = [movie for movie in friend_unique_watched if movie["genre"] == most_watched_genre]| if movie in friend["watched"] and movie in recommendations: | ||
| recommendations.remove(movie) | ||
| elif movie not in friend["watched"] and movie not in recommendations: | ||
| recommendations.append(movie) |
There was a problem hiding this comment.
👀 The two checks here could result in different results depending on which friend is the final one iterated over. If the final friend has watched the user movie, it would be removed (if a previous friend had resulted in adding it), but if the final friend hasn't watched the movie, it could be added in (even if an earlier friend had watched it).
Instead of dealing with uniquing logic again here, we could use one of our earlier wave functions to start from a list of movies only watched by the user.
3 participants
No description provided.