C22, sphinx, Priyanka and Maybellene - #4
Conversation
…atched that passes respective tests
… test and reformats some docstrings
| watched_list = user_data["watched"] | ||
| total_rating = 0 | ||
|
|
||
| if watched_list == []: |
There was a problem hiding this comment.
Hi Ashley,
Sorry about repeating this issue on line 91 and 113
The more Pythonic way would be:
if not lod_watched
Versus what I have here. I hadn't read your feedback when I wrote this
yangashley
left a comment
yangashley
left a comment
There was a problem hiding this comment.
Nice work on viewing-party, Priyanka and Maybellene!
I also see that you two made frequent commits, nice job!
Let me know if you have any questions about my review comments.
| # Act | ||
| new_movie = create_movie(movie_title, genre, rating) | ||
|
|
||
| print(new_movie) |
There was a problem hiding this comment.
Remove debugging print statements before committing code for review
| assert updated_data["watched"] == [{ | ||
| "title": MOVIE_TITLE_1, | ||
| "genre": GENRE_1, | ||
| "rating": RATING_1 | ||
| }] |
| assert updated_data["watchlist"] == [FANTASY_1] | ||
| assert updated_data["watched"] == [FANTASY_2, movie_to_watch] |
There was a problem hiding this comment.
Instead of putting FANTASY_1 in a list and checking that it's equal to updated_data["watchlist"], we could write the assertion on line 193 like:
assert FANTASY_1 in updated_data["watchlist"] How could you write line 194 using the in operator? Also, if the order of the movies in updated_data["watched"] was movie_to_watch and then FANTASY_2, this test would fail. Since we're not concerned with order, using in is a sufficient check.
There was a problem hiding this comment.
would it be:
assert FANTASY2 in updated_data[watched] and movie_to_watch in updated_data[watched]
or
assert for movie in [FANTASY_2, movie_to_watch] if movie in updated_data["watched"]
There was a problem hiding this comment.
Yeah more like your first assertion, but I would break em up on two lines instead of using the and for readability.
assert FANTASY_2 in updated_data["watched"]
assert movie_to_watch in updated_data["watched"] | amandas_data = clean_wave_3_data() | ||
| amandas_data["friends"][0]["watched"].append(INTRIGUE_3) | ||
|
|
||
| expected_movies = [{ |
There was a problem hiding this comment.
expected_movies is created and then isn't accessed in the test so we should remove it from it to keep the test concise.
|
|
||
| # Assert | ||
| assert len(friends_unique_movies) == 3 | ||
| assert INTRIGUE_3 in friends_unique_movies |
There was a problem hiding this comment.
You check that INTRIGUE_3 is in friends_unique_movies. How about explicitly checking that the other two movies that are a part of the movies only watched by friends are also in friends_unique_movies?
| titles_friends_watched = set() | ||
|
|
||
| for friend_movie_dict in user_data["friends"]: | ||
| for movie in friend_movie_dict["watched"]: | ||
| titles_friends_watched.add(movie["title"]) |
There was a problem hiding this comment.
This function currently has 2 responsibilities: getting all the movies friends have watched and then finding unique movies. Prefer this logic to be pulled into a helper function so that it can be called here.
| list_titles_watched = set() | ||
| for movie in user_data["watched"]: | ||
| list_titles_watched.add(movie["title"]) |
There was a problem hiding this comment.
In this function, you build up a list of all the movies a user has watched and then you iterate through all the movies a user's friends have watched to create list_friends_unique_movie.
What if you flipped the logic so that you first get all the movies the friends have watched (like you did in get_unique_watched) and then you can check that the friends' movies list are not in the user's movies list?
If you invert the logic, then you could use reuse the hypothetical helper function that gets friends' movies here too and keep this logic to a single responsibility.
Like this:
def get_friends_unique_watched(user_data):
friends_watched_movies = get_friends_movies()
list_friends_unique_movie = []
for movie in friends_watched_movies:
if movie not in user_data["watched"] and movie not in list_friends_unique_movie:
list_friends_unique_movie.append(movie)
return list_friends_unique_movie| } | ||
| ''' | ||
|
|
||
| lod_unique_friends_movie = get_friends_unique_watched(user_data) |
There was a problem hiding this comment.
Nice job invoking a method you already called to keep your code DRY
| lod_unique_friends_movie = get_friends_unique_watched(user_data) | ||
|
|
||
| # get user's most freq genre | ||
| user_freq_genre = get_most_watched_genre(user_data) |
| "friends": [ {"watched": [...{},{}...] }, {"watched": [...{},{}...] }] | ||
| } | ||
| ''' | ||
| # list of movies only the user has seen (not any of the friends) |
There was a problem hiding this comment.
Since you're using docstrings throughout the project, I'd prefer this function's docstring to have more details about what the function returns instead of having this detail as an inline comment in the function.
3 participants
No description provided.