final viewing_party panthers Kallie_Gannon#169
final viewing_party panthers Kallie_Gannon#169kalliegannon wants to merge 1 commit intoAda-C18:masterfrom
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 ^_^
| assert new_movie is None | ||
|
|
||
| @pytest.mark.skip() | ||
| #this is start of error |
There was a problem hiding this comment.
I'm not sure what code this comment relates to. As part of our code clean up before opening PRs, we should take a look for and remove commented out code and comments/pseudo code we don't need anymore. It helps keep our code concise and reduces possible confusion down the road, especially for other folks working in the same project.
| assert len(updated_data["watchlist"]) == 0 | ||
| assert len(updated_data["watched"]) == 1 | ||
|
|
||
| raise Exception("Test needs to be completed.") |
There was a problem hiding this comment.
We should add our own assertions here after removing the Exception. What information would we need to check to ensure the specific movie from watchlist was moved to the watched list?
| # Assert | ||
| assert len(updated_data["watchlist"]) == 1 | ||
| assert len(updated_data["watched"]) == 2 | ||
| assert updated_data["watched"][1] == movie_to_watch |
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 the movie is in watched, is to check if the entire movie dictionary exists in the list:
assert movie_to_watch in updated_data["watched"]
# Another option:
assert HORROR_1 in updated_data["watched"]Right now the assertions check if movie_to_watch was moved to updated_data["watched"], but what if we want to be sure no other data was affected? We would also want to confirm that the other 2 movies are in the places we expect:
assert HORROR_1 in updated_data["watched"]
assert FANTASY_2 in updated_data["watched"]
assert FANTASY_1 in updated_data["watchlist"]|
|
||
| # Assert | ||
| assert len(friends_unique_movies) == 3 | ||
| assert INTRIGUE_3 in friends_unique_movies |
There was a problem hiding this comment.
There are 3 values in the list friends_unique_movies, we generally would want to confirm all 3 values are what we expect. What assert statements would we need to write to check the other 2 movies in friends_unique_movies?
|
|
||
| def create_movie(title, genre, rating): | ||
| pass | ||
| if title == None or genre == None or rating == None: |
There was a problem hiding this comment.
Nice one-line statement for checking inputs!
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 you’d like more info on this, 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
In this case, we could also rely on the truthy/falsy value of the variables rather than checking for None specifically:
if not title or not genre or not rating:| def get_available_recs(user_data): | ||
| friends_unique_movies = get_friends_unique_watched(user_data) | ||
| rec_movies = [] | ||
| for movies in friends_unique_movies: |
There was a problem hiding this comment.
movies is plural, and there is a convention in programming that a plural name implies that it's a collection holding multiple objects. Since movies will hold a dictionary representing a single movie, I'd recommend using singular movie.
| user_genre = get_most_watched_genre(user_data) | ||
| friends_unique_movies = get_friends_unique_watched(user_data) |
There was a problem hiding this comment.
Great code reuse across waves 4 & 5!
| for movies in friends_unique_movies: | ||
| if movies['genre'] == user_genre: | ||
| rec_list.append(movies) |
There was a problem hiding this comment.
I probably wouldn't recommend doing this in shared code because of how long the line gets and how many expressions we have to parse on one line, but as an example of how powerful list comprehensions can be, we could create and assign rec_list to its final contents in a single line:
rec_list = [movies if movies['genre'] == user_genre for movies in friends_unique_movies:]| fav_rec_list = [] | ||
| friends_unique_movies = get_friends_unique_watched(user_data) | ||
|
|
||
| for movie in get_unique_watched(user_data): | ||
| if movie not in friends_unique_movies: | ||
| fav_rec_list.append(movie) | ||
| print(fav_rec_list) | ||
| return fav_rec_list |
There was a problem hiding this comment.
This implementation is passing the test test_unique_rec_from_favorites, but there's an issue. This approach will return every item in get_unique_watched(user_data) whether or not they are in the list user_data['favorites']. How could you update this function to consider the user's favorite list?
| fav_rec_list = [] | ||
| friends_unique_movies = get_friends_unique_watched(user_data) | ||
|
|
||
| for movie in get_unique_watched(user_data): | ||
| if movie not in friends_unique_movies: | ||
| fav_rec_list.append(movie) |
There was a problem hiding this comment.
get_unique_watched(user_data) will only return movies that our friends have not seen, so we could factor out the variable friends_unique_movies:
fav_rec_list = []
for movie in get_unique_watched(user_data):
fav_rec_list.append(movie)2 participants
No description provided.