C17 - Celina#121
Conversation
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Good job getting your first project in!
Most of your tests are passing, and overall I'm seeing good thinking about code structure and naming. And great use of set functions for working with the unique movies! Nice job!
It looks like you started to get a little tripped on on the logic in wave 4. We can implement the wave 4 and 5 functions by duplicating a lot of logic from wave 3, but that makes those functions quite long, complex, and difficult to think about. Look for similarities in the function descriptions to the wave 3 functions and think about whether we can reuse any of them to make the later functions a little easier to approach!
Overall, this is heading in the right direction.
|
|
||
| assert len(updated_data["watchlist"]) == 0 | ||
| assert len(updated_data["watched"]) == 1 | ||
| assert updated_data["watched"][0]["title"] == MOVIE_TITLE_1 |
|
|
||
| assert len(updated_data["watchlist"]) == 1 | ||
| assert len(updated_data["watched"]) == 2 | ||
| assert movie_to_watch in updated_data["watched"] |
| py==1.10.0 | ||
| pyparsing==2.4.7 | ||
| pytest==6.2.1 | ||
| pytest==6.2.5 |
There was a problem hiding this comment.
👍
Keep an eye out for this on your future python projects as well.
| assert FANTASY_4 in friends_unique_movies | ||
| assert HORROR_1 in friends_unique_movies | ||
| assert INTRIGUE_3 in friends_unique_movies |
| from logging.handlers import WatchedFileHandler | ||
| from tkinter import W |
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.
| fantasy_count = 0 | ||
| action_count = 0 | ||
| intrigue_count = 0 |
There was a problem hiding this comment.
We should not assume that these are the only possible genres. In the existing data (though not in this test) we've also seen horror films, and there could be others (romcom, thriller, comedy, childrens, etc). So we want to be able to tally up the count of each genre regardless of how many distinct genre values there are.
What data structure lets us store something like the genre (a string) along with a value (the count of that genre)? And how could the logic below be rewritten to make use of it?
| for unique_movie in user_unique_watched_set: | ||
| for movie in user_data["watched"]: | ||
| if movie["title"] == unique_movie: | ||
| user_unique_watched_list.append(movie) |
There was a problem hiding this comment.
Since we have a set of unique titles, we can make use of its very efficient membership lookup to avoid the nested loop here. Something like
for movie in user_data["watched"]:
if movie["title"] in user_unique_watched_set:
user_unique_watched_list.append(movie)If we were concerned that there may have been duplicates within the user's own list, we could remove each title we match from the set so that it only gets used once:
for movie in user_data["watched"]:
if movie["title"] in user_unique_watched_set:
user_unique_watched_list.append(movie)
user_unique_watched_set.remove(move["title"])|
|
||
| user_watched_set = set(user_watched_list) | ||
| friend_watched_set = set(friend_watched_list) | ||
| user_unique_watched_set = user_watched_set.difference(friend_watched_set) |
There was a problem hiding this comment.
Great use of set operations. Since sets only allow immutable data, we had to use a unique(ish) immutable part of a move to stand in for the whole record.
| for unique_movie in friend_unique_watched_set: | ||
| for friend in user_data["friends"]: | ||
| for movie in friend["watched"]: | ||
| if movie not in friend_unique_watched_list: | ||
| if movie["title"] == unique_movie: | ||
| friend_unique_watched_list.append(movie) |
There was a problem hiding this comment.
The general approach looks fine, but you're right, we could turn this around a bit, similar to get_unique_watched above.
for friend in user_data["friends"]:
for movie in friend["watched"]:
if movie["title"] in friend_unique_watched_set:
friend_unique_watched_list.append(movie)
friend_unique_watched_set.remove(movie["title"])| # ----------------------------------------- | ||
|
|
||
| def get_available_recs(user_data): | ||
| friend_watched_movie_title_list = [] |
There was a problem hiding this comment.
It looks like a lot of the logic here is similar to the get unique methods from wave 3. Can we reuse those functions to help simplify the implementation here? Part of the description says that the recommendations should be things the user hasn't watched, but that at least one friend has watched. Does that sound like the results of one of the wave 3 functions?
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Thanks for updating this. Looks good! Take a look at the comments throughout for additional pointers to start thinking about in future projects!
| recommendations = get_new_rec_by_genre(sonyas_data) | ||
|
|
||
| # Assert | ||
| assert len(recommendations) == 0 |
|
|
||
| return watched_avg_rating | ||
|
|
||
| def get_most_watched_genre(user_data): |
| # ------------- WAVE 4 -------------------- | ||
| # ----------------------------------------- | ||
|
|
||
| def get_available_recs(user_data): |
| most_watched_genre = get_most_watched_genre(user_data) | ||
| friend_unique_watched = get_friends_unique_watched(user_data) |
| for movie in friends_unique_watched_list: | ||
| for subscription in user_data["subscriptions"]: | ||
| if subscription == movie["host"]: | ||
| available_recommended_movies_list.append(movie) |
There was a problem hiding this comment.
Looking for the movie host in the subscriptions could be written like this:
for movie in friends_unique_watched_list:
if movie["host"] in user_data["subscriptions"]:
available_recommended_movies_list.append(movie)This is exactly equivalent to your version, but replaces the explicit loop with an in check expressing the intent of seeing whether the host is in the subscriptions. The other thing this lets us think about doing is possibly calculate a set of the subscriptions and use that for the in check instead. in checks on sets are much more efficient than for lists, so often, if we have a list that we're making in checks on (especially if it's in a loop) we might consider reorganizing the list as a set for improved performance:
subscription_set = set(user_data["subscriptions"])
for movie in friends_unique_watched_list:
if movie["host"] in subscription_set:
available_recommended_movies_list.append(movie)This version only needs to iterate once to make the set (outside the loop) a d the inside the loop, it uses fast in checks.
| recommended_movies = [] | ||
|
|
||
| for movie in friend_unique_watched: | ||
| if movie not in recommended_movies and movie["genre"] == most_watched_genre: |
There was a problem hiding this comment.
Here, recommended_movies is a list because we want to generate a list of movies for the result. We can't simply make it a set, since dictionaries can't go in a set. But much like your "unique" methods from wave 3, we could make a set of movie titles! Before adding a movie to the result, check whether its title is in the title set. Then if not, add the movie to the result and its title to the title set!
| recommended_movies.append(movie) | ||
| return recommended_movies | ||
|
|
||
| def get_rec_from_favorites(user_data): |
There was a problem hiding this comment.
👍
There are a few in checks here which we could think about addressing either by reorganizing the order of the looping, or by making helper sets of titles as described above.
2 participants
wave 1,2,3 passing