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.
| assert len(updated_data["watched"]) == 1 | ||
|
|
||
| raise Exception("Test needs to be completed.") | ||
| #raise Exception("Test needs to be completed.") |
There was a problem hiding this comment.
The assertion above assert len(updated_data["watched"]) == 1 tells us that there is only one element in the watched list, but it does not guarantee that the element holds the movie data we expect. Whenever we see one of these raise Exception statements in a test file, we need to add our own assertions to complete the test.
What assertions could you add that would confirm the title, genre, and rating of the item in the watched list are what we expect?
There was a problem hiding this comment.
I don't want to flood you with duplicate comments so I want to say here, it looks like all the tests with raise Exception("Test needs to be completed.") are missing new assertions. I recommend for your own practice to go through and try out adding assertions to complete these tests to practice syntax for tests and thinking about what data is necessary to check in order to confirm a scenario.
| if title == None or genre == None or rating == None: | ||
| return None |
There was a problem hiding this comment.
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 NoneIf 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
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|
|
||
| def create_movie(title, genre, rating): | ||
| pass | ||
| movie = dict() |
There was a problem hiding this comment.
I suggest moving the movie declaration under the invalid data check so that we don't allocate space for movie unless we know it will be used.
| else: | ||
| movie["title"] = title | ||
| movie["genre"] = genre | ||
| movie["rating"] = rating | ||
| return movie |
There was a problem hiding this comment.
Since it doesn't change the result of our function, I recommend removing the else so we can outdent the main flow of the function.
If an else clause doesn't change the flow of our code, then we have a statement that needs to be compiled and evaluated, but that has no benefit. Additionally, they might cause the main flow of our code to be indented, when we generally want that to be prominent to readers. I generally recommend always questioning if we need an else clause, and only adding one if it's required.
| def add_to_watched(user_data, movie): | ||
| user_data["watched"] = [] | ||
| user_data["watched"].append(movie) | ||
| return user_data |
There was a problem hiding this comment.
Something we need to be really careful about is avoiding overwriting data that we want to preserve. In the description for the add_to_watched function, the README says:
the value of user_data will be a dictionary with a key "watched", and a value which is a list of dictionaries representing the movies the user has watched
This means that the function parameter user_data is already a dictionary that contains a watched list, and which might have values inside. Line 14 overwrites the user_data's watched list by assigning it to an empty list, losing any data that was in the argument.
How could we re-write this to preserve the data we get in the input user_data?
| user_set = user_watched(user_data) | ||
| friend_set = friends_watched(user_data) | ||
| unique_title = get_difference(user_set, friend_set) |
There was a problem hiding this comment.
I love the use of helper functions to help organize the code!
| for title in unique_title: | ||
| for movie in user_data["watched"]: | ||
| if title == movie["title"]: | ||
| unique_watched.append(movie) |
There was a problem hiding this comment.
We could simplify our loops a little bit by looping over user_data["watched"] and inside the loop checking if the current movie's title is in non_watch_set:
for movie in user_data["watched"]:
if movie["title"] in unique_title:
unique_watched.append(movie)| for title in unique_title: | ||
| for movie in user_data["friends"]: | ||
| for watched in movie["watched"]: | ||
| if title == watched["title"] and watched not in user_has_not_watched: | ||
| user_has_not_watched.append(watched) |
There was a problem hiding this comment.
Similar to the function above, we could remove one of the loops here to simplify our syntax and reduce how much we are looping over the data. What could the code for that change look like?
| # ----------------------------------------- | ||
|
|
||
| def get_available_recs(user_data): | ||
| user_has_not_watched = get_friends_unique_watched(user_data) |
| recommended_movies = list() | ||
|
|
||
| for movie in user_has_not_watched: | ||
| if movie["host"] in user_data["subscriptions"]: | ||
| recommended_movies.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 user_has_not_watched 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 user_has_not_watched
if movie["host"] in user_data["subscriptions"]]
|
Thank you for your comments! I adjusted the code and added assertions in
the test cases. Can you check them out?
…On Fri, Nov 11, 2022 at 2:29 PM Kelsey Steven ***@***.***> wrote:
***@***.**** 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
<#5 (comment)>:
> @@ -119,12 +119,12 @@ def test_moves_movie_from_watchlist_to_empty_watched():
assert len(updated_data["watchlist"]) == 0
assert len(updated_data["watched"]) == 1
- raise Exception("Test needs to be completed.")
+ #raise Exception("Test needs to be completed.")
The assertion above assert len(updated_data["watched"]) == 1 tells us
that there is only one element in the watched list, but it does not
guarantee that the element holds the movie data we expect. Whenever we see
one of these raise Exception statements in a test file, we need to add
our own assertions to complete the test.
What assertions could you add that would confirm the title, genre, and
rating of the item in the watched list are what we expect?
------------------------------
In viewing_party/party.py
<#5 (comment)>:
> + if title == None or genre == None or rating == None:
+ return 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
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
<#5 (comment)>:
> @@ -1,23 +1,148 @@
# ------------- WAVE 1 --------------------
def create_movie(title, genre, rating):
- pass
+ movie = dict()
I suggest moving the movie declaration under the invalid data check so
that we don't allocate space for movie unless we know it will be used.
------------------------------
In viewing_party/party.py
<#5 (comment)>:
> + else:
+ movie["title"] = title
+ movie["genre"] = genre
+ movie["rating"] = rating
+ return movie
Since it doesn't change the result of our function, I recommend removing
the else so we can outdent the main flow of the function.
If an else clause doesn't change the flow of our code, then we have a
statement that needs to be compiled and evaluated, but that has no benefit.
Additionally, they might cause the main flow of our code to be indented,
when we generally want that to be prominent to readers. I generally
recommend always questioning if we need an else clause, and only adding
one if it's required.
------------------------------
In viewing_party/party.py
<#5 (comment)>:
> +def add_to_watched(user_data, movie):
+ user_data["watched"] = []
+ user_data["watched"].append(movie)
+ return user_data
Something we need to be really careful about is avoiding overwriting data
that we want to preserve. In the description for the add_to_watched
function, the README says:
the value of user_data will be a dictionary with a key "watched", and a
value which is a list of dictionaries representing the movies the user has
watched
This means that the function parameter user_data is already a dictionary
that contains a watched list, and which might have values inside. Line 14
overwrites the user_data's watched list by assigning it to an empty list,
losing any data that was in the argument.
How could we re-write this to preserve the data we get in the input
user_data?
------------------------------
In viewing_party/party.py
<#5 (comment)>:
> + user_data["watchlist"] = []
+ user_data["watchlist"].append(movie)
The comment in add_to_watched applies here as well.
------------------------------
In viewing_party/party.py
<#5 (comment)>:
> + #index = user_data["watchlist"].index(data)
+ #user_data["watchlist"].pop(index)
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
<#5 (comment)>:
> + user_data["watched"].append(movie)
+ return user_data
+
+def add_to_watchlist(user_data, movie):
+ user_data["watchlist"] = []
+ user_data["watchlist"].append(movie)
+ return user_data
+
+def watch_movie(user_data, title):
+ for data in user_data["watchlist"]:
+ if title == data["title"]:
+ user_data["watched"].append(data)
+ #index = user_data["watchlist"].index(data)
+ #user_data["watchlist"].pop(index)
+ user_data["watchlist"].remove(data)
+ return user_data
Nice, I like the return in the loop so we stop iterating over the list
after making changes.
------------------------------
In viewing_party/party.py
<#5 (comment)>:
> + average_rating = 0.0
+ rating = 0.0
Similar to feedback in create_movie, I suggest moving the creation of
variables until after the validation check.
------------------------------
In viewing_party/party.py
<#5 (comment)>:
> + for movie in user_data["watched"]:
+ rating += movie["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']`
rating = sum(movie["rating"] for movie in user_data["watched"])
average_rating = rating / len(user_data["watched"])
return average_rating
------------------------------
In viewing_party/party.py
<#5 (comment)>:
>
# -----------------------------------------
# ------------- WAVE 2 --------------------
# -----------------------------------------
+def get_watched_avg_rating(user_data):
+ average_rating = 0.0
+ rating = 0.0
To me, the name rating implies the variable holds a rating for a single
item, rather than a sum of many ratings. What names could better reflect
the data rating holds?
------------------------------
In viewing_party/party.py
<#5 (comment)>:
> + for movie in user_data["watched"]:
+ if movie["genre"] in genre_frequently:
+ genre_frequently[movie["genre"]] += 1
+ else:
+ genre_frequently[movie["genre"]] = 1
+
+ if max < genre_frequently[movie["genre"]]:
+ max = genre_frequently[movie["genre"]]
+ most_watched_genre = movie["genre"]
+ return most_watched_genre
Neat implementation to find the max genre in a single loop over the input!
This doesn't perform better, but another approach could be to use the max
function after filling the dictionary genre_frequently to get the genre
name we want.
genres = dict()
for movie in user_data["watched"]:
if movie["genre"] in genre_frequently:
genre_frequently[movie["genre"]] += 1
else:
genre_frequently[movie["genre"]] = 1
most_watched_genre = max(genre_frequently, key=genre_frequently.get)
return most_watched_genre
------------------------------
In viewing_party/party.py
<#5 (comment)>:
> + for movie in user_data["watched"]:
+ if movie["genre"] in genre_frequently:
+ genre_frequently[movie["genre"]] += 1
+ else:
+ genre_frequently[movie["genre"]] = 1
There's a tradeoff of using a little more space for another variable, but
to make the code a little easier to read and reduce how many times we need
to access movie["genre"], I might suggest creating a variable before the
if that holds the result of movie_entry["genre"] which we can use on the
subsequent lines:
for movie in user_data["watched"]:
movie_genre = movie["genre"]
if movie_genre in genre_frequently:
genre_frequently[movie_genre] += 1
else:
genre_frequently[movie_genre] = 1
------------------------------
In viewing_party/party.py
<#5 (comment)>:
> + user_set = user_watched(user_data)
+ friend_set = friends_watched(user_data)
+ unique_title = get_difference(user_set, friend_set)
I love the use of helper functions to help organize the code!
------------------------------
In viewing_party/party.py
<#5 (comment)>:
> + for title in unique_title:
+ for movie in user_data["watched"]:
+ if title == movie["title"]:
+ unique_watched.append(movie)
We could simplify our loops a little bit by looping over
user_data["watched"] and inside the loop checking if the current movie's
title is in non_watch_set:
for movie in user_data["watched"]:
if movie["title"] in unique_title:
unique_watched.append(movie)
------------------------------
In viewing_party/party.py
<#5 (comment)>:
> + for title in unique_title:
+ for movie in user_data["friends"]:
+ for watched in movie["watched"]:
+ if title == watched["title"] and watched not in user_has_not_watched:
+ user_has_not_watched.append(watched)
Similar to the function above, we could remove one of the loops here to
simplify our syntax and reduce how much we are looping over the data. What
could the code for that change look like?
------------------------------
In viewing_party/party.py
<#5 (comment)>:
> # -----------------------------------------
# ------------- WAVE 4 --------------------
# -----------------------------------------
+def get_available_recs(user_data):
+ user_has_not_watched = get_friends_unique_watched(user_data)
Nice code reuse!
------------------------------
In viewing_party/party.py
<#5 (comment)>:
> + recommended_movies = list()
+
+ for movie in user_has_not_watched:
+ if movie["host"] in user_data["subscriptions"]:
+ recommended_movies.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 user_has_not_watched 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 user_has_not_watched
if movie["host"] in user_data["subscriptions"]]
------------------------------
In tests/test_wave_01.py
<#5 (comment)>:
> @@ -119,12 +119,12 @@ def test_moves_movie_from_watchlist_to_empty_watched():
assert len(updated_data["watchlist"]) == 0
assert len(updated_data["watched"]) == 1
- raise Exception("Test needs to be completed.")
+ #raise Exception("Test needs to be completed.")
I don't want to flood you with duplicate comments so I want to say here,
it looks like all the tests with raise Exception("Test needs to be
completed.") are missing new assertions. I recommend for your own
practice to go through and try out adding assertions to complete these
tests to practice syntax for tests and thinking about what data is
necessary to check in order to confirm a scenario.
—
Reply to this email directly, view it on GitHub
<#5 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKRTJIQCH3KAIP5RFLPZQATWH2M73ANCNFSM6AAAAAARYTVYKQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
The updates look good, nice work! |
2 participants
No description provided.