-
Notifications
You must be signed in to change notification settings - Fork 117
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
Otters - Roshni #111
base: master
Are you sure you want to change the base?
Otters - Roshni #111
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.
Hi Roshni! Your submission has been scored as green. Please check your code to see the comments I left. Let me know if you have any questions. Nice work! :)
|
||
if title == None or genre == None or rating == None: | ||
new_movie = None | ||
return new_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.
This is a good implementation of this function, but something to think about now that we have discussed time complexity is how much work is being done - Does your function have varying outcomes? If so, what's the least amount of work that can be done for each scenario?
In this case, we have 2 scenarios: The inputs are all valid and we create a dictionary or we need to return None. In the case that the input is not valid, we do not need to create a dictionary. Think about how you can refactor this code so that there is no unnecessary work being done.
"rating": rating | ||
} | ||
|
||
if title == None or genre == None or rating == 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.
Small note - when checking if a value is none, it's better to use var is None
rather than var == None
:)
|
||
|
||
def watch_movie(user_data, title): | ||
for movie in user_data["watchlist"]: |
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 super clean solution! Only note is to make sure you're paying attention to how many times your loop will iterate and when it exits. Right now, your loop will keep going until it reaches the end of watchlist even if it already found the title. This is a suuuuper easy mistake to make. Now, that we have chatted about Big O, pay attention to if your code is doing unnecessary work and think about how you could refactor this code to stop looping once the movie is found.
length = len(user_data["watched"]) | ||
rating = 0.0 | ||
|
||
if length == 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.
Love that you get this edge case out the way early so that there is no unnecessary work being done. Beautiful solution ✨
def get_most_watched_genre(user_data): | ||
most_watched = None | ||
|
||
if len(user_data["watched"]) == 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.
Small note but the Pythonic way to write this would be if not user_data["watched"]
since Python evaluates empty lists to False
# Consider the movies that the user has watched, and consider the movies that their friends have watched. | ||
# Determine which movies at least one of the user's friends have watched, but the user has not watched. | ||
# Return a list of dictionaries, that represents a list of movies | ||
def get_friends_unique_watched(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.
Same comments from get_unique_watched()
apply here too. See if you can get this solution down to 2 for loops instead of 4.
# At least one of the user's friends has watched | ||
# The "host" of the movie is a service that is in the user's "subscriptions" | ||
# Return the list of recommended movies | ||
def get_available_recs(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.
Beautiful solution ✨
# The movie is in the user's "favorites" | ||
# None of the user's friends have watched it | ||
# Return the list of recommended movies | ||
def get_rec_from_favorites(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.
This is a great solution! Now I challenge you to see if you can refactor this down to 2 for loops instead of 3. 😄
# else: | ||
# genres[movie["genre"]] += 1 | ||
|
||
if not movie["genre"] in genres: |
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.
Another if-else block that would be a great place to implement a try-except statement to adhere to EAFP (Easier to Ask Forgiveness than Permission). Oooor you could look into the get method to handle keys that don't exist. Here's a resource for that here.
else: | ||
genres[movie["genre"]] += 1 | ||
|
||
most_frequently_watched_genre = max(genres, key=genres.get, default=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.
Hmmmm seems like there might be an existing function that could do this work for us (hint hint get_most_watched_genre()
) :)
No description provided.