-
Notifications
You must be signed in to change notification settings - Fork 632
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
C18 Panthers Class[ Brooke Hill ] #26
base: main
Are you sure you want to change the base?
Conversation
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.
Hey brooke, here is the feedback we walked through during our 1:1. More is coming! Let me know if you have any questions.
from ast import If | ||
from types import NoneType |
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 don't think you need these imports. Please remove these floaty, random bits in future PRs.
|
||
|
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.
One space is sufficient to create 'sections' of logic within functions.
movie = {} | ||
|
||
movie["title"] = movie_title | ||
movie["genre"] = genre | ||
movie["rating"] = rating | ||
|
||
for item in movie.values(): | ||
if item == None: | ||
return None | ||
else: | ||
return 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.
if not movie_title or not genre or not rating:
return None
return {
"title": movie_title,
"genre": genre,
"rating": rating
}
user_data = { | ||
"watched": [] | ||
} | ||
copy = movie.copy() |
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 we want to copy the nested items within a nested data structure, we should use deepcopy
. copy
only copies the outer layer.
def watch_movie(user_data, title): | ||
|
||
|
||
|
||
for movie in user_data["watchlist"]: | ||
if title == movie['title']: | ||
user_data["watched"].append(movie) | ||
user_data["watchlist"].remove(movie) | ||
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.
Brooke, mind your space! Please take up space everywhere in life, except in your functions LOL. One line of blank space is enough.
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.
LOL I wrote this with you during our 1:1 and we were giggling together but reading this afterward is WOWW.... so sassy 🤣 I swear I'm not this blunt LOL
if sum != 0: | ||
avg = sum / count | ||
else: | ||
avg = 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.
if sum == 0:
return 0.0
avg = sum / count
def get_most_watched_genre(user_data): | ||
# will come back to this one to handle all test cases | ||
for movie in user_data["watched"]: | ||
if movie["genre"] == movie["genre"]: | ||
return movie["genre"] | ||
elif len(user_data["watched"]) == 0: | ||
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.
Hey Brooke, revisit this and I'll provide feedback 😄
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.
Approaches we discussed:
- Creating a counter based off of the genres and whichever has the higher count return that
- Creating a counter and a
most_freq
with each iteration increase count and most_freq - (BEST approach, audrey highly suggests) Use a frequency map! Create a dictionary with the genres as a key and the values as their 'count' aka frequency.
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 Brooke! You hit all the learning goals for this project and all tests are passing. You have earned a well-deserved 🟢 grade on this project 🐈⬛ ✨
I added comments to your PR primarily on ways to refactor your code and improve readability. In the future, I'd like to see you use more guard clauses and docstrings in your functions. Docstrings are a great way of leaving notes to your future self about your approach to writing these functions.
Feel free to refactor and improve on this project and do let me know if you want me to take another look for fun 😄
Keep up the great work! ✨
assert updated_data["watched"][0]["title"] == MOVIE_TITLE_1 | ||
assert updated_data["watched"][0]["genre"] == GENRE_1 | ||
assert updated_data["watched"][0]["rating"] == RATING_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.
👍 We don't need to add spaces between these asserts
assert updated_data["watched"][0]["title"] == MOVIE_TITLE_1 | |
assert updated_data["watched"][0]["genre"] == GENRE_1 | |
assert updated_data["watched"][0]["rating"] == RATING_1 | |
assert updated_data["watched"][0]["title"] == MOVIE_TITLE_1 | |
assert updated_data["watched"][0]["genre"] == GENRE_1 | |
assert updated_data["watched"][0]["rating"] == RATING_1 |
# Assert | ||
assert len(updated_data["watchlist"]) == 1 | ||
assert len(updated_data["watched"]) == 2 | ||
|
||
raise Exception("Test needs to be completed.") | ||
#raise Exception("Test needs to be completed.") |
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 you're missing an assert for this test but no worries, we can go over it during a 1:1.
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.
👍 Good work utilizing the in
operator. Also, no need for spaces here as well.
recommendations = get_new_rec_by_genre(sonyas_data) | ||
#raise Exception("Test needs to be completed.") | ||
assert len(recommendations) == 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.
👍
def add_to_watchlist(user_data, movie): | ||
user_data = { | ||
"watchlist": [] | ||
} | ||
copy1 = movie.copy() | ||
|
||
user_data["watchlist"].append(copy1) | ||
|
||
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.
👍
A few suggestions:
copy1
is a very generic name. I recommend using a more descriptive name likewatchlist_copy
.- In the first line of the function, you're actually reassigning
user_data
to be a brand new dictionary with an emptywatchlist
. The watchlist is actually already provided via theuser_data
parameter.
# Add genre counts to a dictionary with value being how many times the genre appears in a list. | ||
for genre in list_of_genres: | ||
genres[genre] = genres.get(genre,0) + 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.
Nice use of get
!
# getting highest value from dict | ||
new_val = genres.values() | ||
if len(new_val) > 0: | ||
max_val = max(new_val) | ||
for genre in genres: | ||
if genres[genre] == max_val: | ||
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.
We can reuse get_most_watched_genre
as a helper function here.
for movie in user_data["friends"][0]["watched"]: | ||
friends_list.append(movie) | ||
for movie in user_data["friends"][1]["watched"]: | ||
friends_list.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.
Same comment as in the previous function about considering scalability.
for movies in friends_list: | ||
if movies not in user_data["watched"] and movies["genre"] == most_watched_genre: | ||
new_recs.append(movies) | ||
return new_recs |
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_rec_from_favorites(user_data): | ||
user_favorites = [] | ||
friend_list = [] | ||
movie_recs = [] | ||
|
||
|
||
for movies in user_data["favorites"]: | ||
user_favorites.append(movies) | ||
if len(user_data["friends"]) > 0: | ||
for friends in user_data["friends"][0]["watched"]: | ||
friend_list.append(friends) | ||
for friends in user_data["friends"][1]["watched"]: | ||
friend_list.append(friends) | ||
|
||
for movies in user_favorites: | ||
if movies not in friend_list: | ||
movie_recs.append(movies) | ||
return movie_recs |
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 work! We can use a guard clause here to allow the logic for building friend_list
to be less indented.
def get_rec_from_favorites(user_data):
user_favorites = []
friend_list = []
movie_recs = []
if not user_data or not user_data["friends"]:
return
for movies in user_data["favorites"]:
user_favorites.append(movies)
for friends in user_data["friends"][0]["watched"]:
friend_list.append(friends)
for friends in user_data["friends"][1]["watched"]:
friend_list.append(friends)
for movies in user_favorites:
if movies not in friend_list:
movie_recs.append(movies)
return movie_recs
No description provided.