Open
Conversation
kelsey-steven-ada
left a comment
kelsey-steven-ada
left a comment
There was a problem hiding this comment.
🎉 Congrats on completing your first project at Ada! 🎉 I’ve added some suggestions & questions, let me know if there's anything I can clarify.
Comment on lines
+121
to
+123
| 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.
Great checks for all the movie data!
| # Assert | ||
| assert len(updated_data["watchlist"]) == 1 | ||
| assert len(updated_data["watched"]) == 2 | ||
| assert updated_data["watched"][1]["title"] == "It Came from the Stack Trace" |
There was a problem hiding this comment.
The README isn't explicit about what order we should add movies to the list watched. If we want our tests to be independent from a specific implementation that adds movies to a particular location in the watched list, we could use the in keyword to check if the variable movie_to_watch is in the watched list instead of looking at a specific index. By checking for the whole dictionary, there's a side benefit that we don't need to check each key independently:
assert movie_to_watch in updated_data["watched"]
# Another option:
assert HORROR_1 in updated_data["watched"]| assert len(friends_unique_movies) == 3 | ||
|
|
||
| raise Exception("Test needs to be completed.") | ||
| #raise Exception("Test needs to be completed.") |
There was a problem hiding this comment.
We're missing adding new asserts for this test. What assertions could you make to ensure the exact movies that we expect are in the list friends_unique_movies?
Comment on lines
+9
to
+43
| ''' | ||
| #-----WAVE 3-------- | ||
|
|
||
| USER_DATA_2 = { | ||
| "watched": [ | ||
| FANTASY_1, | ||
| FANTASY_2, | ||
| FANTASY_3, | ||
| ACTION_1, | ||
| INTRIGUE_1, | ||
| INTRIGUE_2 | ||
| ], | ||
| } | ||
| USER_DATA_3 = copy.deepcopy(USER_DATA_2) | ||
|
|
||
| USER_DATA_3["friends"] = [ | ||
| { | ||
| "watched": [ | ||
| FANTASY_1, | ||
| FANTASY_3, | ||
| FANTASY_4, | ||
| HORROR_1, | ||
| ] | ||
| }, | ||
| { | ||
| "watched": [ | ||
| FANTASY_1, | ||
| ACTION_1, | ||
| INTRIGUE_1, | ||
| INTRIGUE_3, | ||
| ] | ||
| } | ||
| ] | ||
|
|
||
| ''' |
There was a problem hiding this comment.
It's all good to add commented out test data like this while we're actively working on tests, but we want to remove it before committing and pushing our changes to keep our test code concise and uncluttered.
Comment on lines
+56
to
+59
| recommendations = get_new_rec_by_genre(sonyas_data) | ||
|
|
||
| # Assert | ||
| assert len(recommendations) == 0 |
| #print(friends_movie_list) | ||
| my_unique_watched_list = [] | ||
| for my_unique_movie in user_data["watched"]: | ||
| if my_unique_movie not in friends_movie_list and my_unique_movie not in my_unique_watched_list: |
There was a problem hiding this comment.
This line gets a bit long. We might want to drop it over a couple lines or create variables to hold the comparisons that we then use in an if statement:
is_movie_in_friends = my_unique_movie in friends_movie_list
is_movie_in_unique_list = my_unique_movie in my_unique_watched_list
if not is_movie_in_friends and not is_movie_in_unique_list:
my_unique_watched_list.append(my_unique_movie)| return my_unique_watched_list | ||
| ###+++++++++++++++++++++ | ||
|
|
||
| def get_friends_unique_watched(user_data): |
|
|
||
| ###### Wave 4++++++++++++++++++++++++++++ | ||
| def get_available_recs(user_data): | ||
| friends_unique_watched_movies = get_friends_unique_watched(user_data) |
Comment on lines
+89
to
+92
| recommended_movies_for_me = [] | ||
| for movie in friends_unique_watched_movies: | ||
| if movie["host"] in user_data["subscriptions"]: | ||
| recommended_movies_for_me.append(movie) |
There was a problem hiding this comment.
Nice algorithm! Another way we could approach filtering the unique_watched list is with a list comprehension:
# list comprehension
result = [movie for movie in friends_unique_watched_movies if movie["host"] in user_data["subscriptions"]]This line is a bit long, in practice we would split a statement like this across lines, use some extra variables, or shorten some naming to keep under the PEP8 guide of 79 characters max per line:
# list comprehension
result = [movie for movie in friends_unique_watched_movies
if movie["host"] in user_data["subscriptions"]]
Comment on lines
+108
to
+111
| friends_movie_list = [] | ||
| for friend in user_data["friends"]: | ||
| for movie in friend["watched"]: | ||
| friends_movie_list.append(movie) |
There was a problem hiding this comment.
It looks like we duplicate some code that creates a list of our friends movies between get_unique_watched and this function. How could we use a helper function to remove the duplication?
Author
|
Hi Kelsey,
Thank you so much for all the comments. I will make sure I follow the suggested practices in my coding.
Also should I make these changes and push the code to git?
Supriya
… On Nov 11, 2022, at 11:28 AM, Kelsey Steven ***@***.***> wrote:
@kelsey-steven-ada commented on this pull request.
🎉 Congrats on completing your first project at Ada! 🎉 I’ve added some suggestions & questions, let me know if there's anything I can clarify.
In tests/test_wave_01.py <#9 (comment)>:
> + 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
Great checks for all the movie data!
In tests/test_wave_01.py <#9 (comment)>:
> @@ -142,13 +144,14 @@ def test_moves_movie_from_watchlist_to_watched():
# Assert
assert len(updated_data["watchlist"]) == 1
assert len(updated_data["watched"]) == 2
+ assert updated_data["watched"][1]["title"] == "It Came from the Stack Trace"
The README isn't explicit about what order we should add movies to the list watched. If we want our tests to be independent from a specific implementation that adds movies to a particular location in the watched list, we could use the in keyword to check if the variable movie_to_watch is in the watched list instead of looking at a specific index. By checking for the whole dictionary, there's a side benefit that we don't need to check each key independently:
assert movie_to_watch in updated_data["watched"]
# Another option:
assert HORROR_1 in updated_data["watched"]
In tests/test_wave_03.py <#9 (comment)>:
> @@ -55,12 +90,12 @@ def test_friends_unique_movies_not_duplicated():
# Assert
assert len(friends_unique_movies) == 3
- raise Exception("Test needs to be completed.")
+ #raise Exception("Test needs to be completed.")
We're missing adding new asserts for this test. What assertions could you make to ensure the exact movies that we expect are in the list friends_unique_movies?
In tests/test_wave_03.py <#9 (comment)>:
> + '''
+#-----WAVE 3--------
+
+USER_DATA_2 = {
+ "watched": [
+ FANTASY_1,
+ FANTASY_2,
+ FANTASY_3,
+ ACTION_1,
+ INTRIGUE_1,
+ INTRIGUE_2
+ ],
+}
+USER_DATA_3 = copy.deepcopy(USER_DATA_2)
+
+USER_DATA_3["friends"] = [
+ {
+ "watched": [
+ FANTASY_1,
+ FANTASY_3,
+ FANTASY_4,
+ HORROR_1,
+ ]
+ },
+ {
+ "watched": [
+ FANTASY_1,
+ ACTION_1,
+ INTRIGUE_1,
+ INTRIGUE_3,
+ ]
+ }
+ ]
+
+ '''
It's all good to add commented out test data like this while we're actively working on tests, but we want to remove it before committing and pushing our changes to keep our test code concise and uncluttered.
In tests/test_wave_05.py <#9 (comment)>:
> + recommendations = get_new_rec_by_genre(sonyas_data)
+
+ # Assert
+ assert len(recommendations) == 0
Great act & assert steps!
In viewing_party/party.py <#9 (comment)>:
>
-# -----------------------------------------
-# ------------- WAVE 2 --------------------
-# -----------------------------------------
+ if title == None or genre == None or rating == None:
Nice handling to return None for missing data! In Python, when we test if a variable is pointing to None the PEP8 style guide recommends using is or is not for comparison rather than operators like ==.
None is a special object in Python, it’s a singleton, meaning there is only ever one instance of None in memory. All variables holding None are actually pointing to the same instance. By using is to compare against None, we avoid issues that could be caused by a class’s custom implementation of the comparison operators, and we gain some speed over a check using == since we are checking against identity rather than looking up if the variable's held type has a comparison function that should be used.
if title is None or genre is None or rating is None :
return None
If you’d like more info on is vs == for None, here’s a couple resources to get started =]
https://peps.python.org/pep-0008/#programming-recommendations <https://peps.python.org/pep-0008/#programming-recommendations>
http://jaredgrubb.blogspot.com/2009/04/python-is-none-vs-none.html <http://jaredgrubb.blogspot.com/2009/04/python-is-none-vs-none.html>
Something else to consider: here we're explicitly checking if the values are None, but what if we got an empty string for title? If we wanted to check for empty strings 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:
return None
In viewing_party/party.py <#9 (comment)>:
>
+ movie_dict = {
+ "title": title ,
+ "genre": genre ,
+ "rating": rating
+ }
+ return movie_dict
+
+#++++++++
+def add_to_watched(user_data, movie):
+ user_data["watched"].append(movie)
+ print(user_data)
Print statements are wonderful to help us while writing or debugging code, but if a problem statement doesn't ask us to print some particular data, we want remove any print statements we added as part of our review and clean up before opening a PR.
In viewing_party/party.py <#9 (comment)>:
> + for movie in user_data["watchlist"]:
+ if movie["title"] == title:
+ user_data["watchlist"].remove(movie)
+ user_data["watched"].append(movie)
+ return user_data
This function works, but as an aside it contains an "anti-pattern" that is important to be aware of. An element of the list that is being iterated over is being removed inside the for loop. This won't cause problems in this particular case, but consider this slightly different implementation:
def watch_movie(user_data, title):
for i in range(len(user_data["watchlist"])):
item = user_data["watchlist"][i]
if item["title"] == title:
user_data["watched"].append(item)
user_data["watchlist"].remove(item)
return user_data
If we were to test this in a scenario where user_data["watchlist"] has 2 elements, and the first is the movie that we are moving to user_data["watched"], the second iteration through the loop will throw an index error, because it will try to access the 2nd element in user_data["watchlist"], but there no longer is a second element.
As a coding pattern, this is one that I recommend avoiding because it has so much potential for unforeseen errors. One fix is to return immediately after making the swap (ie, add return user_data inside the if-block so that the loop ends as soon as the swap is made). Another fix is to use the loop to find the item, but not make the swap until after the loop ends.
In viewing_party/party.py <#9 (comment)>:
> + "genre": genre ,
+ "rating": rating
+ }
+ return movie_dict
+
+#++++++++
+def add_to_watched(user_data, movie):
+ user_data["watched"].append(movie)
+ print(user_data)
+ return user_data
+#++++++++++++
+def add_to_watchlist(user_data, movie):
+ user_data["watchlist"].append(movie)
+ print(user_data)
+ return user_data
+#+++++++++++++++
The PEP8 style guide doesn't say much about blank lines between functions when they aren't part of a class, but I would suggest following the guideline for functions in a class here by using a single blank line to give readers a visual separation between functions, removing the comment separators.
https://peps.python.org/pep-0008/#blank-lines <https://peps.python.org/pep-0008/#blank-lines>
In viewing_party/party.py <#9 (comment)>:
> + return user_data
+#++++++++++++
+def add_to_watchlist(user_data, movie):
+ user_data["watchlist"].append(movie)
+ print(user_data)
+ return user_data
+#+++++++++++++++
+def watch_movie(user_data, title):
+ for movie in user_data["watchlist"]:
+ if movie["title"] == title:
+ user_data["watchlist"].remove(movie)
+ user_data["watched"].append(movie)
+ return user_data
+###### WAve 2#################
+def get_watched_avg_rating(user_data):
+ if len(user_data["watched"]) == 0:
I'd suggest using the truthy/falsy value of user_data["watched"] which will return false if the list is empty or points to None:
if not user_data["watched"]:
return 0.0
In viewing_party/party.py <#9 (comment)>:
> + total = 0
+ for movie in user_data["watched"]:
+ total += movie["rating"]
+ avg_rating = total / len(user_data["watched"])
+ return avg_rating
Nice approach! Another way we could do this is using list comprehensions and the sum function:
# The list comprehension inside the `sum` call pulls the value of the
# `rating` key out of each item in the list `user_data['watched']`
total = sum(movie["rating"] for movie in user_data["watched"])
avg_rating = total / len(user_data["watched"])
return avg_rating
In viewing_party/party.py <#9 (comment)>:
> + popular_genre_freq = 0
+ for genre in genre_their_freq_in_userdata:
+ genre_freq = genre_their_freq_in_userdata[genre]
+ if genre_freq > popular_genre_freq:
+ popular_genre = genre
+ popular_genre_freq = genre_freq
Since lines 53-58 are inside of the for loop that start on line 48, we make a full loop over genre_their_freq_in_userdata and recalculate the most popular genre with every movie in user_data["watched"]. There's a few approaches we could try to reduce how much we need to iterate over data:
One option is to move lines 53-58 outside of the for loop for movie in user_data["watched"]: and calculate popular_genre once at the very end.
Another option is to declare popular_genre and popular_genre_freq before the loop for movie in user_data["watched"]:, then replace lines 53-58 with something like:
if genre_their_freq_in_userdata[this_genre] > popular_genre_freq:
popular_genre_freq = genre_their_freq_in_userdata[genre]
popular_genre = this_genre
The last approach I want to touch on would involve removing lines 53-58, then after the loop for movie in user_data["watched"]: we could use the build in max function to get the genre we want from genre_their_freq_in_userdata:
for movie in user_data["watched"]:
this_genre = movie["genre"]
if this_genre not in genre_their_freq_in_userdata.keys():
genre_their_freq_in_userdata[this_genre] = 1
else:
genre_their_freq_in_userdata[this_genre] += 1
popular_genre = max(genre_their_freq_in_userdata, key=genre_their_freq_in_userdata.get)
return popular_genre
In viewing_party/party.py <#9 (comment)>:
> # ------------- WAVE 3 --------------------
-# -----------------------------------------
+def get_unique_watched(user_data):
As our functions grow, they can become dense blocks of text that are harder to quickly parse and understand. I suggest adding blank lines sparingly inside your functions to separate chunks of code that perform slightly different tasks. In this function I might add a blank line to visually separate the chunk of code that creates and fills friends_movie_list from the chunk of code that iterates over user_data["watched"] to fill my_unique_watched_list.
In viewing_party/party.py <#9 (comment)>:
> # ------------- WAVE 3 --------------------
-# -----------------------------------------
+def get_unique_watched(user_data):
+ friends_movie_list = []
+ for friends_watched_movie in user_data["friends"]:
+ for movie in friends_watched_movie["watched"]:
+ friends_movie_list.append(movie)
+ #print(friends_movie_list)
We want to remove commented out code before opening PRs, especially in shared projects, so that we don't bring things into the codebase that could be confusing down the road. If we have been making commits as we work, we can always check out the code at those commits if we want to take a look at something we've since changed or deleted.
In viewing_party/party.py <#9 (comment)>:
> # ------------- WAVE 3 --------------------
-# -----------------------------------------
+def get_unique_watched(user_data):
+ friends_movie_list = []
+ for friends_watched_movie in user_data["friends"]:
+ for movie in friends_watched_movie["watched"]:
+ friends_movie_list.append(movie)
+ #print(friends_movie_list)
+ my_unique_watched_list = []
+ for my_unique_movie in user_data["watched"]:
+ if my_unique_movie not in friends_movie_list and my_unique_movie not in my_unique_watched_list:
This line gets a bit long. We might want to drop it over a couple lines or create variables to hold the comparisons that we then use in an if statement:
is_movie_in_friends = my_unique_movie in friends_movie_list
is_movie_in_unique_list = my_unique_movie in my_unique_watched_list
if not is_movie_in_friends and not is_movie_in_unique_list:
my_unique_watched_list.append(my_unique_movie)
In viewing_party/party.py <#9 (comment)>:
> -# -----------------------------------------
+def get_unique_watched(user_data):
+ friends_movie_list = []
+ for friends_watched_movie in user_data["friends"]:
+ for movie in friends_watched_movie["watched"]:
+ friends_movie_list.append(movie)
+ #print(friends_movie_list)
+ my_unique_watched_list = []
+ for my_unique_movie in user_data["watched"]:
+ if my_unique_movie not in friends_movie_list and my_unique_movie not in my_unique_watched_list:
+ my_unique_watched_list.append(my_unique_movie)
+ #print(my_unique_watched_list)
+ return my_unique_watched_list
+###+++++++++++++++++++++
+
+def get_friends_unique_watched(user_data):
Nice implementation!
In viewing_party/party.py <#9 (comment)>:
> + #print(my_unique_watched_list)
+ return my_unique_watched_list
+###+++++++++++++++++++++
+
+def get_friends_unique_watched(user_data):
+ friends_unique_movie = []
+ for friend in user_data["friends"]:
+ for movie in friend["watched"]:
+ if movie not in user_data["watched"] and movie not in friends_unique_movie:
+ friends_unique_movie.append(movie)
+ #print(friends_unique_movie)
+ return friends_unique_movie
+
+###### Wave 4++++++++++++++++++++++++++++
+def get_available_recs(user_data):
+ friends_unique_watched_movies = get_friends_unique_watched(user_data)
Great code reuse!
In viewing_party/party.py <#9 (comment)>:
> + recommended_movies_for_me = []
+ for movie in friends_unique_watched_movies:
+ if movie["host"] in user_data["subscriptions"]:
+ recommended_movies_for_me.append(movie)
Nice algorithm! Another way we could approach filtering the unique_watched list is with a list comprehension:
# list comprehension
result = [movie for movie in friends_unique_watched_movies if movie["host"] in user_data["subscriptions"]]
This line is a bit long, in practice we would split a statement like this across lines, use some extra variables, or shorten some naming to keep under the PEP8 guide of 79 characters max per line:
# list comprehension
result = [movie for movie in friends_unique_watched_movies
if movie["host"] in user_data["subscriptions"]]
In viewing_party/party.py <#9 (comment)>:
> + friends_movie_list = []
+ for friend in user_data["friends"]:
+ for movie in friend["watched"]:
+ friends_movie_list.append(movie)
It looks like we duplicate some code that creates a list of our friends movies between get_unique_watched and this function. How could we use a helper function to remove the duplication?
—
Reply to this email directly, view it on GitHub <#9 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB2LLGNKQ37TPRDZWVLXKETWH2M7RANCNFSM6AAAAAAR26QQ6Y>.
You are receiving this because you authored the thread.
|
|
Since the project is "Green" on the grading scale (the green/yellow/red grade is shared in Learn), you do not need to make any changes, but you are always welcome to if there is something you want to practice =] |
Author
|
Yes I want to and need to practice a lot so will definitely work on making it better with all your suggestions. Thank you. Supriya Sent from my iPhoneOn Nov 11, 2022, at 12:22 PM, Kelsey Steven ***@***.***> wrote:
Since the project is "Green" on the grading scale (the green/yellow/red grade is shared in Learn), you do not need to make any changes, but you are always welcome to if there is something you want to practice =]
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.