Conversation
kelsey-steven-ada
left a comment
kelsey-steven-ada
left a comment
There was a problem hiding this comment.
Congrats on your first project! 🎉 I’ve added some suggestions & questions, please let me know if there's anything I can clarify ^_^
| pp.pprint(HORROR_1) | ||
| pp.pprint(FANTASY_1) | ||
| pp.pprint(FANTASY_2) | ||
| # print("\n-----Wave 01 test data-----") |
There was a problem hiding this comment.
Committing changes to this file & the README is no issue for this project, but something to think about going forward is that we generally don't want to commit temporary changes for testing, or changes to files that we don't intend to purposefully share with other folks. This can mean having to be more selective when staging files for commit over being able to use git add .
| 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.
Nice checks for all the movie's data!
| assert updated_data["watched"][1]["title"] == MOVIE_TITLE_1 | ||
| assert updated_data["watched"][1]["genre"] == GENRE_1 | ||
| assert updated_data["watched"][1]["rating"] == RATING_1 |
There was a problem hiding this comment.
Our problem statement doesn't specify what order a movie should be added to the watched list in. If movies were added to the beginning to the list rather than the end, these tests would fail. A more flexible way we could check if a movie is in watched, is to check if the entire movie dictionary exists in the list:
assert HORROR_1 in updated_data["watched"]When writing tests, we want to check all the relevant data to ensure there weren't unexpected side affects. Since there are other movies in the watchlist and watched lists, we could also add assertions for those:
assert FANTASY_2 in updated_data["watched"]
assert FANTASY_1 in updated_data["watchlist"]| if not title or not genre or not rating: | ||
| return None |
There was a problem hiding this comment.
Nice one-liner to validate all the inputs!
In this implementations, the movies variable will be created and allocated space before we know if the inputs are valid. I would recommend flipping the order and placing the validity check first so that movies is only created if it will be needed.
|
|
||
| def create_movie(title, genre, rating): | ||
| pass | ||
| movies = { |
There was a problem hiding this comment.
When we pluralize a variable name, that usually means the variable is pointing to a collection holding many objects. If movies isn't a collection of multiple movies, we probably want to give it a singular name to better reflect what it holds.
| for my_movies in my_watched: | ||
| cool_movies_watched.append(my_movies) |
There was a problem hiding this comment.
Python's list comprehensions give us another option to fill a list from existing data:
# We can read the list comprehension on the right side of the assignment operator as:
# "For each movie in user_data['watched'] add movie to a new list"
cool_movies_watched = [movie for movie in user_data['watched']]| for friends_watched in friends: | ||
| for friends_movies in friends_watched['watched']: | ||
| friends_mediocre_movies.append(friends_movies) |
There was a problem hiding this comment.
This doesn't change the function's overall performance, but another option to add values from one list to another is the extend function:
for friends_watched in friends:
friends_mediocre_movies.extend(friends_watched['watched')| return unique_movies | ||
|
|
||
|
|
||
| def get_friends_unique_watched(user_data): |
There was a problem hiding this comment.
The feedback for get_unique_watched applies to this function as well.
| for movie in cool_movies_watched: | ||
| if not movie in friends_mediocre_movies and movie not in unique_movies: | ||
| unique_movies.append(movie) | ||
| return unique_movies |
There was a problem hiding this comment.
Great approach with nested loops! Another of many possible implementations could use the Set data structure to solve this – if we created a Set that held our user_data["watched"] movies and another Set that held all the movies in our friends' watched lists, then we could take the difference of those sets!
my_movies = set(user_data["watched"])
friend_movies = set()
for friend in user_data["friends"]:
for movie in friend["watched"]:
friend_movies.add(movie)
unique_watched_set = my_movies - friend_movies
return list(unique_watched_set)| for friends_watched in friends: | ||
| for friends_movies in friends_watched['watched']: | ||
| friends_mediocre_movies.append(friends_movies) |
There was a problem hiding this comment.
We have some repeated code in get_unique_watched and here to fill a list with all our friend's movies. How could a helper function reduce this repeated code?
2 participants
No description provided.