-
Notifications
You must be signed in to change notification settings - Fork 81
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
Zoisite - Katherine G and Johanna C #53
base: main
Are you sure you want to change the base?
Conversation
Merge branch 'main' of https://github.com/johanna-j-c/viewing-party
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.
GREEN! This project is really well done. Y'all have a great handle on nested data structures and nested loops! Y'all also did a great job using the helper functions you created. I left a few suggestions on refactoring, but overall it looks great!
movie_dict["title"]= title | ||
movie_dict["genre"]= genre | ||
movie_dict["rating"]= rating | ||
return movie_dict |
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 job with this function! As we've kind of talked about a bit in class, if we can get away without using an else statement, that's ideal. Especially when it comes to guard clauses. If we have a guard clause that returns None, then the else is essentially implied and not necessary!
def watch_movie(user_data, title): | ||
for movie in user_data["watchlist"]: | ||
if movie["title"] == title: | ||
user_data["watched"].append(movie) |
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.
Not really a problem, but a challenge to think about: we created a helper function that adds a movies to the watchlist! I know it's just a one line function, but we should definitely get into the habit of using our helper functions where we can!
user_data["watched"].append(movie) | ||
user_data["watchlist"].remove(movie) | ||
return user_data | ||
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 use of returning the user_data in both the if statement and outside of the loop in case the movie is never found!
def get_watched_avg_rating(user_data): | ||
sum = 0.0 | ||
if not user_data["watched"]: | ||
return sum |
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.
This is a well constructed guard clause! One small tweak is that you could add an "or" condition to see if the length of user_data == 0. Our tests did not go there, but it's possible that the watched list exists, it's just empty! Great job checking for this however as it does avoid the 0 division!
for movie in user_data["watched"]: | ||
sum += movie["rating"] | ||
average = sum / len(user_data["watched"]) | ||
return average |
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.
The rest of this function looks great!
recommendation_list = [] | ||
|
||
|
||
for friend_dict in friend_unique: |
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.
Just be careful when naming variables. "friend_dict" feels like it's looking at specific friends in "friend_unique", when in reality we are looking at each movie in "friend_unique"!
for friend_dict in friend_unique: | ||
if friend_dict["host"] in user_subscription_list: | ||
recommendation_list.append(friend_dict) | ||
return(recommendation_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.
The rest of this looks great!
for friend_dict in friend_unique: | ||
if friend_dict["genre"] == user_most_watched_genre: | ||
recommendation_list.append(friend_dict) | ||
return recommendation_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.
Once again, just be careful with the naming. Names should clearly state what is being held and "friend_dict" is just slightly confusing to someone looking through your code, but overall this looks really good! I love your use of helper functions! Great catch!
user_unique = get_unique_watched(user_data) | ||
recommendation_list = [] | ||
|
||
for favorite in user_data["favorites"]: |
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.
"favorite" is a much better name choice for this loop!
for favorite in user_data["favorites"]: | ||
if favorite in user_unique: | ||
recommendation_list.append(favorite) | ||
return recommendation_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.
Once again, great job using helper functions. This is a really good implementation of the get_rec_from_favorites!
No description provided.