Skip to content

Sphinx-Denise-Ajar-Lorraine #1

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

Open
wants to merge 28 commits into
base: main
Choose a base branch
from
Open

Conversation

shiyinxi
Copy link

No description provided.

@shiyinxi shiyinxi changed the title Sphinx-Lorraine Shi Sphinx-Denise-Aja-Lorraine Sep 26, 2024
@shiyinxi shiyinxi changed the title Sphinx-Denise-Aja-Lorraine Sphinx-Denise-Ajar-Lorraine Sep 26, 2024
Comment on lines 8 to 9
new_movie_info = {"title": title, "genre": genre, "rating": rating}
return new_movie_info

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we aren't really doing anything with new_movie_info besides returning it, I would be okay with just returning the dictionary directly like so:

return {"title": title, "genre": genre, "rating": rating}

Copy link

@dpchengmath dpchengmath Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed and pushed!

for movie in user_data["watchlist"]:
if movie["title"] == title:
user_data["watchlist"].remove(movie)
user_data["watched"].append(movie)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this line, did we write a function that could do this for us? I know it's trivial here, but it helps to get in the habit so we can keep our code DRY. Also, why write a function if we won't use it? ✨ Great work on this function overall!

Copy link

@dpchengmath dpchengmath Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed and pushed!

Comment on lines 20 to 22
for movie in user_data["watchlist"]:
if movie["title"] == title:
user_data["watchlist"].remove(movie)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So now that we have talked about Big O analysis, we know that .remove has a time complexity of O(n) since it needs to look through every element in a list to find the sought after value. Right now, this loop has an outer loop of O(n) and you when you find the movie to remove you have another O(n) operation. How could we circumvent using remove? (Hint: Think about list comprehension)

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: will ask about it in office hours

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed this thanks for explaining!

Comment on lines 28 to 39
def get_watched_avg_rating(user_data):
if not user_data["watched"]:
return 0

total_movie_ratings = 0

for movie_rating in user_data["watched"]:
total_movie_ratings += movie_rating["rating"]

average_movie_rating = total_movie_ratings / len(user_data["watched"])

return average_movie_rating

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

for single_movie in user_data["watched"]:
genre_type = single_movie.get("genre")
if genre_type:
frequency_of_genre_index[genre_type] = frequency_of_genre_index.get(genre_type,0) + 1

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love this line right here! A great example of using the .get method that dictionaries supply us with!

Comment on lines +50 to +52
if frequency_of_genre_index[genre_type] > max_frequency:
max_frequency = frequency_of_genre_index[genre_type]
most_watched_genre = genre_type
Copy link

@mikellewade mikellewade Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work on this logic, I now that you've done it a few times (adagrams and PSE 1) I think it would be okay to use the max function that python provides us.

EDIT: Discussing with other instructors we came to the conclusion that we think that it's best to not use max until Unit 2 or to use it and have the logic written out and commented out so we know that you understand the logic behind it.

Comment on lines 62 to 63
my_friends_unique = [movie["title"] for my_friends in user_data["friends"]
for movie in my_friends["watched"]]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job with the list comprehension here, I would say this is the most I would try to fit in a comprehension. I think that anything pass this would lower the readability of your code.

Comment on lines +65 to +66
my_unique_movies = [movie for movie in user_data["watched"]
if movie["title"] not in my_friends_unique]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since my_friends_unique is a list, the in operator has an O(n) time complexity here. Is there another data structure that we can use to circumvent the complexity here? How would that change our code?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: will ask about it in office hours

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A set can take a constant amount of time to find the elements. A list would have to look at n-1.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in my_unique_movies....

if movie["title"] not in my_friends_unique]

searching through the my_friends_unique variable would take a constant if it was a set rather than if it was a list.

if movie["title"] not in my_movie_titles and movie not in friends_unique_movies:
friends_unique_movies.append(movie)

return list(friends_unique_movies)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Being that friends_unique_movies is a list already, is there a reason that we wrap it in list()?

Copy link

@dpchengmath dpchengmath Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed and pushed!


def get_available_recs(user_data):
recommended_movies = []
friends_unique_movies = get_friends_unique_watched(user_data)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using functions that you've already written! Keeping your code DRY! 🎉

Comment on lines 89 to 91
user_watched_titles = set()

user_watched_titles = [movie['title'] for movie in user_data.get('watched', [])]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why this was declared as a set and then converted into a list? I would actually say to keep it as a set since it could improve the efficiency of your code.

Copy link

@dpchengmath dpchengmath Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed and pushed!

Comment on lines 95 to 98
recommended_movies = [movie for movie in friends_unique_movies
if movie['host'] in user_subscriptions
and movie['title'] not in user_watched_titles
if movie not in recommended_movies]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this is a good example of maybe opting to not us list comprehension, you lose out on readability here. Even though not using comprehension takes us a little longer to type out, both ways function the same. We would re-gain the loss readability we lost here.

Copy link

@dpchengmath dpchengmath Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed and pushed!

Comment on lines 104 to 110
def get_new_rec_by_genre(user_data):
most_watched_genre = get_most_watched_genre(user_data)

recommended_movie = [movie for movie in get_friends_unique_watched(user_data)
if movie["genre"] == most_watched_genre]

return recommended_movie

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, the only amendment I might ofter is to have result of get_friends_unique_watched assigned to a variable for readability.

Copy link

@dpchengmath dpchengmath Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed and pushed!

Comment on lines +118 to +119
recommended_movie = [movie for movie in user_data["favorites"]
if movie["title"] not in my_friends_watched]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the use of a set here! Gives you a constant look up time!

Comment on lines +162 to +169
assert updated_data == {
"watchlist": [],
"watched": [{
"title": MOVIE_TITLE_1,
"genre": GENRE_1,
"rating": RATING_1
}]
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +195 to +200
assert updated_data == {
"watchlist": [
FANTASY_1
],
"watched": [FANTASY_2, HORROR_1]
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏿

recommendations = get_new_rec_by_genre(sonyas_data)

# Assert
assert recommendations == []

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏿

Copy link

@mikellewade mikellewade left a 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants