Ocelots - Lisa Utsett#11
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.") | ||
| assert updated_data["watched"][0]['title'] == MOVIE_TITLE_1 |
There was a problem hiding this comment.
Nice assertion for the title! In this case where the test is checking individual keys of a dictionary, I recommend including assertions for all of the relevant keys.
watched_movie = updated_data["watched"][0]
assert watched_movie["title”] == MOVIE_TITLE_1
assert watched_movie["rating"] == RATING_1
assert watched_movie["genre"] == GENRE_1| assert len(updated_data["watched"]) == 2 | ||
|
|
||
| raise Exception("Test needs to be completed.") | ||
| assert updated_data["watched"][1]['title'] == movie_to_watch["title"] |
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 | ||
| assert len(friends_unique_movies) == 3 | ||
| assert friends_unique_movies.count(INTRIGUE_3) == 1 |
There was a problem hiding this comment.
Nice check for INTRIGUE_3 👍 I would suggest checking for each of the movies we expect in friends_unique_movies to ensure only what we expected was added to the list.
assert INTRIGUE_3 in friends_unique_movies
assert HORROR_1 in friends_unique_movies
assert FANTASY_4 in friends_unique_movies| recommendations = get_new_rec_by_genre(sonyas_data) | ||
|
|
||
| # Assert | ||
| assert len(recommendations) == 0 |
|
|
||
| def create_movie(title, genre, rating): | ||
| pass | ||
| if not title or not genre or not rating: |
There was a problem hiding this comment.
Great use of the truthy/falsy values to check for empty strings or None in one statement!
| unique_movies = [] | ||
| user_movies = [] | ||
|
|
||
| for user_movie in user_data["watched"]: | ||
| user_movies.append(user_movie["title"]) | ||
|
|
||
| for friend in user_data["friends"]: | ||
| for movie in friend["watched"]: | ||
| if movie["title"] not in user_movies and movie not in unique_movies: | ||
| unique_movies.append(movie) | ||
|
|
||
| return unique_movies |
There was a problem hiding this comment.
There is nothing wrong with this organization, but I want to suggest a slight change that can help readers, especially in longer functions. Instead of declaring all our variables at the top of a function, I recommend creating them just before you need to use them. This help reduce needing to jump around the function or scroll up the page to remind ourselves of what a variable holds.
user_movies = []
for user_movie in user_data["watched"]:
user_movies.append(user_movie["title"])
unique_movies = []
for friend in user_data["friends"]:
...| user_movies = [] | ||
|
|
||
| for user_movie in user_data["watched"]: | ||
| user_movies.append(user_movie["title"]) |
There was a problem hiding this comment.
If we don't need complicated logic to transform data, a common way to fill a list in Python is using a list comprehension:
user_movies = [movie["title"] for movie in user_data["watched"]]Here's a resource on list comprehensions if you want more info: https://realpython.com/list-comprehension-python/
|
|
||
| def get_available_recs(user_data): | ||
| friends_recs = [] | ||
| friends_unique_data = get_friends_unique_watched(user_data) |
| for movie in friends_unique_data: | ||
| if movie["host"] in user_data["subscriptions"]: | ||
| friends_recs.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_data 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_data
if movie["host"] in user_data["subscriptions"]]| rec_by_genre = [] | ||
| most_watched_genre = get_most_watched_genre(user_data) | ||
| recs = get_available_recs(user_data) | ||
| for movie in recs: | ||
| if movie["genre"] == most_watched_genre: | ||
| rec_by_genre.append(movie) | ||
|
|
||
| return rec_by_genre |
There was a problem hiding this comment.
Though this solution passes the current test suite, it isn't doing what the problem statement asks.
The test test_new_genre_rec uses clean_wave_5_data() as the input, which includes subscription data. If we remove "hulu" from the user's subscriptions on line 157 of test_constants.py, suddenly the test test_new_genre_rec starts failing. Our problem statement for get_new_rec_by_genre does not ask us to consider subscriptions though, so changing that subscription data should not change the result of get_new_rec_by_genre.
- What steps can you take to diagnose what is happening?
- What was the cause of the issue?
2 participants
No description provided.