-
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
Sea Turtles - Esther Annorzie - Viewing Party #112
base: master
Are you sure you want to change the base?
Conversation
viewing_party/party.py
Outdated
return user_data | ||
|
||
|
||
def watch_movie(user_data, MOVIE_TITLE_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.
MOVIE_TITLE_1 is the name of one of the data constants for our tests, so we should pick a different name for our parameter. To follow the pep 8 style guide, our parameter name should be all lower-case letters. Some examples might be "movie_title", "title_of_movie", or "title_to_watch".
viewing_party/party.py
Outdated
|
||
|
||
def watch_movie(user_data, MOVIE_TITLE_1): | ||
for movies in user_data.values(): |
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 remove a movie from "watchlist" and move it into into "watched", we only need to loop over the "watchlist" to let us retrieve the full movie details. The outer for-loop is looping over user_data.values(), so there will be two iterations: the first one loops over "watchlist", and the second iteration loops over the "watched" list.
The first iteration of our outer for-loop does exactly what we want: it finds the movie in the "watchlist", removes it from "watchlist", and adds the movie to the "watched" list.
The second iteration is an interesting case. We are now looping over "watched" which is already holding the movie we wanted it to. The inner loop finds the movie, removes it from the "watched" list, then adds it back to the "watched" list!
How could we structure our loops, or what keywords could we use to prevent us from iterating over the "watched" list after we've already moved the movie from one list to the other?
viewing_party/party.py
Outdated
@@ -1,23 +1,151 @@ | |||
# ------------- WAVE 1 -------------------- | |||
|
|||
def create_movie(title, genre, rating): | |||
pass | |||
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.
Nice handling for returning "None" for missing data! In this case we would want to use is
rather than ==
for comparison because we are checking for None
.
When comparing things in python, ==
compares if the values are equivalent, while is
checks if they have the same identity. The ==
operator for objects can also be overloaded (re-defined) while None
is a "Singleton object", an object which only ever has one instance, and thus one ID. Since there is only one instance of None allocated by python, all variables holding None
must be pointing to the same ID in memory. All this put together means:
- When comparing a variable against None
==
could give us the wrong value. - We want to use
is
, because if a variable holds None, it always points to the same ID as theNone
singleton object we refer to when we type outNone
.
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.
Something else we can consider: here we're explicitly checking if the values are None
, but what if we got empty strings for one of the values? If we wanted to check for those at the same time as we check for None
, we could rely on the truthy/falsy values of the parameters and write something like: if not title or not genre or not rating:
assert len(updated_data["watchlist"]) is 0 | ||
assert len(updated_data["watched"]) is 1 | ||
assert len(updated_data["watchlist"]) == 0 | ||
assert len(updated_data["watched"]) == 1 | ||
|
||
# ******************************************************************************************* | ||
# ****** Add assertions here to test that the correct movie was added to "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.
It looks like the step of adding our own assertions was missed. What could we assert about the data for a specific movie if we wanted to confirm that the movie in the "watched" list holds the information that we expect it to?
assert len(updated_data["watchlist"]) is 1 | ||
assert len(updated_data["watched"]) is 2 | ||
assert len(updated_data["watchlist"]) == 1 | ||
assert len(updated_data["watched"]) == 2 | ||
|
||
# ******************************************************************************************* | ||
# ****** Add assertions here to test that the correct movie was added to "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.
Same question as for the test above, what could we assert about the three pieces of data a movie holds to confirm it is the movie we expect it to be?
@@ -59,7 +59,7 @@ def test_friends_unique_movies_not_duplicated(): | |||
# ****** Add assertions here to test that the correct movies are 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.
The assert
that came with the test lets us know how many movies to expect in the list friends_unique_movies
, what can we assert to make sure the data each movie holds is correct?
viewing_party/party.py
Outdated
for movies in user_data.values(): | ||
for movie in movies: | ||
if movie["title"] == MOVIE_TITLE_1: | ||
add_movie = movies.pop(movies.index(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.
We are continuing to iterate after removing a value from a mutable list. This puts us in the danger zone we discussed in the "Iterating over data" roundtable.
After an item is removed from a list that is being looped over, the iterator that gives us the next element for our loop will likely not be pointing where we expect! Some approaches to handle this could be creating a deep copy of the data before we loop and editing the copy instead of the original we are looping over, or by breaking out of the loop after we remove an item (so we do not continue iterating over the altered data).
|
||
# ----------------------------------------- | ||
# ------------- WAVE 2 -------------------- | ||
# ----------------------------------------- | ||
|
||
def get_watched_avg_rating(user_data): | ||
try: |
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 this use of try/except for handling the ZeroDivisionError!
viewing_party/party.py
Outdated
for movies in user_data.values(): | ||
for movie in movies: | ||
most_popular_genre.append(movie['genre']) | ||
user_data = max(set(most_popular_genre), key = most_popular_genre.count) |
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.
user_data
is the name of our function parameter, so I recommend creating a new variable here with a name that describes the kind of value we will be returning to the user.
From a readability perspective, we are currently overwriting the user_data variable with drastically different data than it was holding before. This can make it hard for someone unfamiliar with the code to reason about what that data should be representing and what type of data it might hold at any given time.
viewing_party/party.py
Outdated
most_popular_genre.append(movie['genre']) | ||
user_data = max(set(most_popular_genre), key = most_popular_genre.count) | ||
else: | ||
user_data = 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.
Great choice for clarity to be explicit about what our function returns if the "watched" list is empty!
viewing_party/party.py
Outdated
def get_unique_watched(user_data): | ||
unique_watched = [] | ||
friend_1_watched_movies = user_data["friends"][0]["watched"] | ||
friend_2_watched_movies = user_data["friends"][1]["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.
This works great if we know we will only ever have 2 friends lists to compare to, but say we have 3 friends? Or 10? We want out code to be able to be flexible enough that it can scale to larger inputs without us having to make changes to our code.
Like all coding problems, there's several approaches we could take! One way could be to use nested loops to make sure we touch all the data we need to. Another approach would be to make a helper function that flattens all the movies in the various friend's lists into a single list with no duplicates, then loop over it to check for unique watched movies the same way you are doing below.
viewing_party/party.py
Outdated
friend_2_watched_movies = user_data["friends"][1]["watched"] | ||
|
||
for movie in user_data["watched"]: | ||
if movie in friend_1_watched_movies or movie in friend_2_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.
Sometimes we can factor out part of an if-statement by inverting the logic statement. In this case, the only action in the if-block is to continue to the next iteration, we rely on the else to handle when there is an action to be taken.
What we want to do is add an item to unique_watched
if it is not in either of our friend's lists, which means we could re-write this if/else block as a single if-block:
if movie not in friend_1_watched_movies and movie not in friend_2_watched_movies:
unique_watched.append(movie)
viewing_party/party.py
Outdated
friends_unique_watched = [] | ||
|
||
friend_1_watched_movies = user_data["friends"][0]["watched"] | ||
friend_2_watched_movies = user_data["friends"][1]["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.
The comments on scaling from the get_unique_watched
function also apply here. How can we take this approach and make it more generic so we can handle any number of friends?
viewing_party/party.py
Outdated
if movie in user_data["watched"]: | ||
continue | ||
else: | ||
if movie in friends_unique_watched: | ||
continue | ||
else: | ||
friends_unique_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.
We could flip our logic to simplify our if-statements here as well! Lines 93-99 could be re-written as:
if movie not in user_data["watched"] and movie not in friends_unique_watched:
friends_unique_watched.append(movie)
viewing_party/party.py
Outdated
if movie not in user_data["watched"] and movie["host"] in user_data["subscriptions"]: | ||
movie_recs.append(movie) | ||
else: | ||
continue |
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 like seeing folks practice with loop keywords like continue
!
In this case, we could omit the else
block since we only call continue
in it, and the result would be the same.
viewing_party/party.py
Outdated
for movie in movies_friends_watched: | ||
if movie not in user_data["watched"] and movie["host"] in user_data["subscriptions"]: |
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.
Here's a place where we could leverage some of what we've already written! Instead of making a tuple out of the friend's watched movies and then needing to check inside the loop if the user has not watched the movie, what if we used our get_friends_unique_watched
function?
This would give us a list of unique unwatched movies for our user to loop over, then inside our loop, our if-statement would only need to check on the movie_host.
viewing_party/party.py
Outdated
|
||
# ----------------------------------------- | ||
# ------------- WAVE 5 -------------------- | ||
# ----------------------------------------- | ||
def get_new_rec_by_genre(user_data): | ||
# user_movie_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.
Where do you mapping out your approach or write out your pseudocode before translating it to code for your functions?
I really like how you're using whitespace inside and between your functions to make it easy to see sections of related code, and the way you are breaking down assignments into shorter lines to make things easy to read and follow! I recommend starting to incorporate short comments through your code where they can clarify what a section of code is doing. For example you might not write a comment above a block of variable assignments if they are easy to read and understand, but writing one that states the intention above a block of nested loops or if-statements that can be harder to trace the execution of can be very helpful. |
Solid work Esther, especially in the first few waves! I've added suggestions on how you can refactor some of your code to be more efficient, and we'll take a look at the loops and data access in Wave 4 together. Check out the feedback, and let me know if you have any questions! |
# ****** Complete the Act and Assert Portions of theis tests ********** | ||
# ********************************************************************* |
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.
For this test we need to complete both the act & assert steps. What would that look like?
''' | ||
friends_watched_movies = get_friends_watched_movies(user_data) | ||
user_watched_movies = get_user_watched_movies(user_data) | ||
pass |
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 pass
keyword is a placeholder to make sure the code will compile if there are no other statements - we should delete it once we start to write our functions.
finished waves 1-4