-
Notifications
You must be signed in to change notification settings - Fork 29
Sphinx_Olga_Karaivanska_Rong_Jiang #11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
if title and genre and rating: | ||
return { | ||
"title": title, | ||
"genre": genre, | ||
"rating": rating | ||
} | ||
return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could also see this written as:
def create_movie(title, genre, rating):
if not title or not genre or not rating:
return None
return {
"title": title,
"genre": genre,
"rating": rating
}
def watch_movie(user_data,title): | ||
movie_to_move = None | ||
for movie in user_data['watchlist']: | ||
if movie['title'] == title: | ||
movie_to_move = movie | ||
break | ||
|
||
if movie_to_move: | ||
user_data['watchlist'].remove(movie_to_move) | ||
user_data['watched'].append(movie_to_move) | ||
|
||
return user_data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Love how you used the break
keyword here as well since once you find a movie you don't need to look through the rest of it! Here I want to talk about the remove
method. Remember that remove
has an O(n)
time complexity. There are a couple of ways that we could circumvent this. Here is one way that doesn't change your code as much:
def watch_movie(user_data,title):
index = None
watchlist = user_data["watchlist"]
for i in range(len(watchlist)):
movie = watchlist[i]
if movie['title'] == title:
index = i
break
if index is not None:
movie_to_move = watchlist.pop(index)
add_to_watched(user_data, movie_to_move)
return user_data
Also in this implementation, we will only use remove
once, but in other filtering problems we might have to do this for multiple elements so it doesn't hurt to get in the habit now.
EDIT: I made mistake here in the analysis of the pop
method. It is only O(1)
for removing the last element in a list. Here it would still be O(n)
since the list needs to be shifted. An actual alternative would be:
def watch_movie(user_data, title):
movie_to_watch = None
watchlist = user_data["watchlist"]
watched = user_data["watched"]
new_watchlist = []
for movie in watchlist:
if movie.get("title") == title:
add_to_watched(user_data, movie)
else:
new_watchlist.append(new_watchlist)
user_data["watchlist"] = new_watchlist
return user_data
watched_movies = user_data.get('watched', []) | ||
|
||
rating_sum = 0.0 | ||
total_movies = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to count the number of movies watched or can we use the provided built in len
function here instead? If you were looking to lower your time complexity here, I encourage you to do some research about the len
function.
if total_movies == 0: | ||
return 0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great that you are using guard clauses to guard your function from bad input! Usually these would be found at the top of the function in cases like these. it could look at little something like this:
if not watched_movies:
return 0.0
if genre_count[genre] > max_count: | ||
max_count = genre_count[genre] | ||
most_watched_genre = genre |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤩
if genre not in genre_count: | ||
genre_count[genre] = 1 | ||
else: | ||
genre_count[genre] += 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How could these lines be condensed into one line by using .get
? 👀
watched_movie = user_data.get('watched',[]) | ||
friends = user_data.get('friends',[]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that I would be okay with user_data["watched"]
syntax here. Mainly because the on requirements of the data structure then all instances of user_data
should have keys friends
and watched
. If not, then the caller of the function should take another look at the data structure.
watched_movie = user_data.get('watched',[]) | ||
friends = user_data.get('friends',[]) | ||
|
||
friends_watched_titles = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What other data structures could we use here in order to circumvent the linear time complexity on line 102
? 👀
for movie in watched_movie: | ||
if movie['title'] not in friends_watched_titles: | ||
user_unique_watched_movies.append(movie) | ||
print(user_unique_watched_movies) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, don't forget to remove these!
friends_watched_titles = [] | ||
for friend in friends: | ||
for friend_movie in friend.get('watched', []): | ||
friends_watched_titles.append(friend_movie['title']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love how you flattened this list so you could make your loops more simplified!
friends_watched_movie_list = user_data.get('friends', []) | ||
friends_watched_title_set = set() | ||
|
||
for user_watched_movies in user_watched_movie_list: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could opt for a more succinct variable name here, like movie
instead of user_watched_movies
? This makes me think it is more than one movie.
for firends_watched_movie in friends_movies_list.get('watched', []): #dict.value | ||
if firends_watched_movie['title'] not in user_watched_title_set and firends_watched_movie['title'] not in friends_watched_title_set: | ||
friends_unique_watched_movies_list.append(firends_watched_movie) | ||
friends_watched_title_set.add(firends_watched_movie['title']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see here that you are making use of a set
so that you have a constant look up time, I love that! The only thing I might suggest is that the readability here is not where it could be. What changes to this part of the code could increase readability?
def get_available_recs(user_data): | ||
|
||
result = [] | ||
|
||
user_subscriptions = set(user_data.get('subscriptions', [])) | ||
friends_recommendations = get_friends_unique_watched(user_data) | ||
|
||
for movie in friends_recommendations: | ||
if movie['host'] in user_subscriptions: | ||
result.append(movie) | ||
return result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
def get_new_rec_by_genre(user_data): | ||
recommended_movies_list = [] | ||
|
||
user_most_frequent_genre = get_most_watched_genre(user_data) | ||
friends_unique_watched_movies_list = get_friends_unique_watched(user_data) | ||
|
||
for movie in friends_unique_watched_movies_list: | ||
if movie['genre'] == user_most_frequent_genre: | ||
recommended_movies_list.append(movie) | ||
|
||
return recommended_movies_list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨
if movie['title'] not in friends_watched_movies_title_list: | ||
recommend_movies_list.append(movie) | ||
|
||
return recommend_movies_list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about the requirements of if a movie can be added to the recommend_movies_list
:
- Determine a list of recommended movies. A movie should be added to this list if and only if:
- The movie is in the user's "favorites"
- None of the user's friends have watched it
Don't we have a function that takes care of the second requirement? How could we change our function here so that it utilizes that function?
|
||
@pytest.mark.skip() | ||
# Assert | ||
assert recommendations == [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏿
assert INTRIGUE_3 in friends_unique_movies | ||
assert HORROR_1 in friends_unique_movies | ||
assert FANTASY_4 in friends_unique_movies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
@@ -158,13 +159,13 @@ def test_moves_movie_from_watchlist_to_empty_watched(): | |||
# Assert | |||
assert len(updated_data["watchlist"]) == 0 | |||
assert len(updated_data["watched"]) == 1 | |||
|
|||
raise Exception("Test needs to be completed.") | |||
assert any(movie["title"] == MOVIE_TITLE_1 for movie in updated_data["watched"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice using the any
function to check if the generator contains True
after looping through to see if a movie's title matches MOVIE_TITLE_1
. I think that it might be more safeguarded to check if the entire movie is present in a list rather than title though. Mainly so that it makes sure that a that entire movie dictionary is present.
assert updated_data["watchlist"] == [FANTASY_1] | ||
assert updated_data["watched"] == [FANTASY_2, movie_to_watch] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⭐️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, everyone! Feel free to reach out to me if you have any questions the feedback I left!
No description provided.