AdaC22_phoenix_Marjana_Khatun #24
Conversation
There was a problem hiding this comment.
Solid work overall! There are a few issues in the tests and the waves noted in the comments that I would like you to address when you can.
A note around commit messages for the future: I love to see that you're making many smaller commits as you work! Something I'd like to see you work on as we move forward is to include commit messages that describe what work was done in each commit, like what functions were added, or what code was refactored, etc...
tests/test_wave_01.py
Outdated
| assert len(updated_data["watchlist"]) == 1 | ||
| assert len(updated_data["watched"]) == 0 |
There was a problem hiding this comment.
None of the existing asserts should be altered, only new assertions should be added to the test.
If the test is failing with the original assertions, that means we need to do some debugging to understand why the test is failing.
What new assertions can we add to this test to ensure that no data was unexpectedly changed, such as the contents of the movie dictionary?
tests/test_wave_01.py
Outdated
| assert len(updated_data["watchlist"]) == 2 | ||
| 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.
I would like you to revisit this to revert the changes to the original assertions, and add new assertions that would confirm nothing unexpected has changed.
|
|
||
| @pytest.mark.skip() | ||
| # Assert | ||
| assert len(recommendations) == 0 |
There was a problem hiding this comment.
Nice assertion!
| if not title or not genre or not rating: | ||
| return None | ||
| else: | ||
| movies = {} | ||
| movies["title"] = title | ||
| movies["genre"] = genre | ||
| movies["rating"] = rating | ||
| return movies |
There was a problem hiding this comment.
I recommend only using an else when it's necessary for the code to behave how we want. There are many cases where an else doesn't change the outcome of our code. We typically want the intended path of our code to be as prominent as possible, so if an else isn't changing the final result, we want to remove them to make our intended path as clear as possible.
if not title or not genre or not rating:
return None
movies = {}
movies["title"] = title
movies["genre"] = genre
movies["rating"] = rating
return movies | friend_movies_collection = friend_movies_list(user_data) | ||
|
|
||
| for movie in friend_movies_collection: | ||
| if movie["host"] in user_data["subscriptions"] and movie not in user_data["watched"] and movie not in recomanded_movies: |
There was a problem hiding this comment.
This line is really long, what are some strategies that we could use to split up the line so it stays within the Python PEP8 guideline of 79 characters max?
| for movie in friend_movies_collection: | ||
| if movie["host"] in user_data["subscriptions"] and movie not in user_data["watched"] and movie not in recomanded_movies: | ||
| recomanded_movies.append(movie) |
There was a problem hiding this comment.
Since we need to look for movies that:
- the user has not watched it and
- at least one of the user's friends has watched
how could we reuse our earlier function get_friends_unique_watched to simplify our code here?
| friend_movies_collection = friend_movies_list(user_data) | ||
|
|
||
| for movie in friend_movies_collection: | ||
| if movie not in user_data["watched"] and movie["genre"] in most_genre and movie not in recomanded_movie : |
There was a problem hiding this comment.
How could we reuse our earlier function get_friends_unique_watched to simplify our code here?
| def get_new_rec_by_genre(user_data): | ||
|
|
||
| recomanded_movie = [] | ||
| if not user_data["watched"] or not user_data["friends"]: |
There was a problem hiding this comment.
There are some extra spaces between operators sprinkled across the project. Once we have code working, ensuring our spacing is as-expected is a good checklist step before submitting our code for review.
| if not user_data["watched"] or not user_data["friends"]: | |
| if not user_data["watched"] or not user_data["friends"]: | |
| for movie in user_data["favorites"]: | ||
| favorite_movies.append(movie) | ||
|
|
||
| for movies in favorite_movies: | ||
| if movie not in friend_movies_collection and movie not in recommanded_movies: | ||
| recommanded_movies.append(movie) |
There was a problem hiding this comment.
Are these for-loops intended to be nested?
In each iteration of the outer loop we add one more movie to favorite_movies, then we loop over all of favorite_movies. This means we are checking each movie in favorite_movies many extra times.
What happens if you un-nest these loops? Something I'm noticing that could be confusing to other developers is that the inner loop declares a variable movies that is never used - the if-statement that follows (line 157) uses the movie variable declared in the first loop.
| @@ -183,7 +185,8 @@ def test_moves_movie_from_watchlist_to_watched(): | |||
| # Assert | |||
| assert len(updated_data["watchlist"]) == 1 | |||
| assert len(updated_data["watched"]) == 2 | |||
| #assert len(updated_data) ["watchedlist"] == | |||
| assert movie_to_watch == HORROR_1 | |||
There was a problem hiding this comment.
This confirms that the data we input into the test movie_to_watch holds HORROR_1, but we aren't looking inside the lists that were changed to confirm the elements there are the movie dictionaries that we expect. How could we inspect the contents of the updated_data["watchlist"] and updated_data["watched"] lists?
No description provided.