Viewing Party: Sea Turtles - Hillary Smith #101
Conversation
…t_most_watched_genre to allow more genres
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Nice job!
All your tests are passing, with reasonable additions where required, and your code is nicely structured, and makes good re-use of functions from earlier waves. Nice custom helper function as well!
We'll start looking more at performance and efficiency going ahead, so I included some thoughts there as well, mostly around avoiding in on lists when we might be able to use sets which have better membership lookup performance as long as we can figure out how to represent our data in an immutable way so it can be added to the set.
Well done!
| @@ -1,10 +1,11 @@ | |||
| from turtle import up | |||
There was a problem hiding this comment.
Be careful of VSCode importing stray libraries. When we autocomplete suggestions from VSCode, sometimes we accidentally pick a recommendation to use something not in our code (usually from a typo). When that happens, VSCode might think that it needs to import a library if the suggestion refers to a library not in our code. Even if we fix the typo, the imports will remain! So keep an eye on the import statements, and make sure to remove any that don't look familiar.
| assert updated_data["watched"][0]["title"] is MOVIE_TITLE_1 | ||
| assert updated_data["watched"][0]["genre"] is GENRE_1 | ||
| assert updated_data["watched"][0]["rating"] is RATING_1 |
| # Assert | ||
| assert len(updated_data["watchlist"]) is 1 | ||
| assert len(updated_data["watched"]) is 2 | ||
| assert updated_data["watched"][1] is movie_to_watch |
|
|
||
| # Arrange | ||
| assert len(friends_unique_movies) == 3 | ||
| assert friends_unique_movies[2] is INTRIGUE_3 |
There was a problem hiding this comment.
We should double check what the other two movies are as well (what if INTRIGUE_3 made it into the results twice)? Also, since there's no strongly specified behavior about the order of the results, doing in checks might be preferred. The test should try to avoid making promises that the function implementation might not actually keep.
| # Act | ||
| recommendations = get_new_rec_by_genre(sonyas_data) | ||
|
|
||
| # Assert | ||
| assert len(recommendations) == 0 |
| friend_watched_movies.append(movie["title"]) | ||
|
|
||
| for film in user_data["watched"]: | ||
| if film ["title"] not in friend_watched_movies: |
There was a problem hiding this comment.
Great observation that we can use the title as a stand-in for the entire movie. Notice that a title (string) is also immutable, meaning we could store it in a set for improved in check performance, which is why I made the earlier comments about using a set instead (which we couldn't do if we tried to store an entire movie).
| if movie["title"] not in users_watched_movies\ | ||
| and movie not in friends_unique_movies: | ||
| friends_unique_movies.append(movie) |
There was a problem hiding this comment.
Generally, prefer using implicit line wrapping, which is allowed for any expression wrapped in parentheses. Also, because of the typical indentation of the wrapped line, I often add a blank line between the condition and its block to help visually separate them.
if (movie["title"] not in users_watched_movies
and movie not in friends_unique_movies):
friends_unique_movies.append(movie)There was a problem hiding this comment.
Consider tracking a a helper set of the titles that have been added to the output list. We could use that to perform the uniqueness check rather than the result list itself, which would have greater efficiency at the cost of building an extra data structure.
| # ------------- WAVE 4 -------------------- | ||
| # ----------------------------------------- | ||
|
|
||
| def get_available_recs(user_data): |
There was a problem hiding this comment.
👍
Similar comments as for get_friends_unique_watched apply here as well. Look for other places in the remainder of the project that they also might be useful!
| # ------------- WAVE 5 -------------------- | ||
| # ----------------------------------------- | ||
|
|
||
| def get_new_rec_by_genre(user_data): |
| for movies in user_data["friends"]: | ||
| for movie in movies["watched"]: | ||
| friends_watched_movies.append(movie["title"]) |
There was a problem hiding this comment.
I was a little surprised that you didn't make a helper method for this operation similar to get_users_watched_movie_titles!
2 participants
No description provided.