Bumblebees - Esther Kang, Aigerim Kalygulova#5
Conversation
… refactors get_unique_watched, passes all tests in test_wave_03.py
…adds and passes 2 tests checking no duplicates added
apradoada
left a comment
apradoada
left a comment
There was a problem hiding this comment.
Overall, really well done, y'all! This project will receive a Green! Feel free to reach out if y'all have any questions about your feedback!
| def test_does_not_add_duplicate_movie_to_user_watched(): | ||
| # Arrange | ||
| user_data = { | ||
| "watched": [FANTASY_2] | ||
| } | ||
|
|
||
| # Act | ||
| updated_data = add_to_watched(user_data, FANTASY_2) | ||
|
|
||
| # Assert | ||
| assert len(updated_data["watched"]) == 1 | ||
|
|
There was a problem hiding this comment.
This is a great added test! While we try to cover a variety of potential cases in the tests we have written, there are absolutely times that we just can't cover every edge case. You are always welcome to write more tests that more accurately represent what your code is doing!
That being said (And this is just for info purposes), the tests that get run against your code are just the ones provided, so do make sure any added tests don't break the ones that are already there!
| assert updated_data["watched"][0] == { | ||
| "title": MOVIE_TITLE_1, | ||
| "genre": GENRE_1, | ||
| "rating": RATING_1 | ||
| } |
There was a problem hiding this comment.
What I love about this particular assert statement is how specific it is. We know exactly where in the watched list the movie is going to end up, so instead of just checking to see if it's in the list, you've gone ahead and checked the specific index to see if it is what you are expecting. One small tweak you could make here would be to hold the expected movie in its own variable which would allow for a simpler assert statement. I say you "could" make it because at the end of the day, the approach you use will likely be dictated by the team's best practices.
| assert len(updated_data["watched"]) == 2 | ||
|
|
||
| raise Exception("Test needs to be completed.") | ||
| assert updated_data["watched"][-1] == movie_to_watch |
| def test_if_watchlist_empty_return_unchanged_user_data(): | ||
| # Arrange | ||
| janes_data = { | ||
| "watchlist": [], | ||
| "watched": [FANTASY_2] | ||
| } | ||
|
|
||
| # Act | ||
| updated_data = watch_movie(janes_data, "Movie") | ||
|
|
||
| # Assert | ||
| assert len(updated_data["watchlist"]) == 0 | ||
| assert len(updated_data["watched"]) == 1 |
There was a problem hiding this comment.
I like this added test, but it does look quite similar to the test right before it! Just be careful to make sure you're not duplicating tests! I know that the main difference between the tests is that the watchlist is empty in one. This function should do the same thing whether or not watchlist is empty, so I don't think the extra test is necessary in this case!
| def test_return_first_instance_of_most_watched_when_tied(): | ||
| # Arrange | ||
| tied_data_1 = {"watched": [FANTASY_1, FANTASY_2, ACTION_1, ACTION_2, FANTASY_3, ACTION_3]} | ||
| tied_data_2 = {"watched": [FANTASY_1, FANTASY_2, ACTION_1, ACTION_2, ACTION_3, FANTASY_3]} | ||
|
|
||
| # Act | ||
| popular_genre_1 = get_most_watched_genre(tied_data_1) | ||
| popular_genre_2 = get_most_watched_genre(tied_data_2) | ||
|
|
||
| # Assert | ||
| assert popular_genre_1 == "Fantasy" | ||
| assert popular_genre_2 == "Action" No newline at end of file |
There was a problem hiding this comment.
I really like this test! This is a great example of an edge case that we don't look for but y'all have decided is important to your code! Well done!
| def get_friends_movies_list_no_duplicates(friends_list): | ||
| friends_movies = [] | ||
|
|
||
| for friend in friends_list: | ||
| for movie in friend["watched"]: | ||
| if movie not in friends_movies: | ||
| friends_movies.append(movie) | ||
|
|
||
| return friends_movies |
There was a problem hiding this comment.
I think that overall, getting a collection of all the unique movies for the friends is a great idea. And this function works well.
One small concern though is that the not in step on line 92 does technically make this a triply nested loop which is not ideal. An obvious replacement would be to use sets as they are implemented using Hash maps and allow us to add while avoiding duplicate items in constant time. We do have a slight drawback in the fact that sets do not work with mutable elements (We couldn't add dictionaries to a set for example).
With this in mind, we could create a set of movie titles that could be used later.
*Note that this particular approach would require us to modify the helper function and potentially the functions that call this one as well!
| for movie_1 in list_1: | ||
| is_unique = True | ||
|
|
||
| for movie_2 in list_2: | ||
| if movie_1 == movie_2: | ||
| is_unique = False | ||
| break | ||
|
|
||
| if is_unique: | ||
| unique_movies.append(movie_1) |
There was a problem hiding this comment.
Putting my previous suggestion about working with sets instead of lists aside, consider using the in method here as opposed to a nested loop!
| for movie_1 in list_1: | |
| is_unique = True | |
| for movie_2 in list_2: | |
| if movie_1 == movie_2: | |
| is_unique = False | |
| break | |
| if is_unique: | |
| unique_movies.append(movie_1) | |
| for movie_1 in list_1: | |
| if movie_1 not in list_2: | |
| unique_movies.append(movie_1) | |
| recommendations = [] | ||
| friends_unique_movies = get_friends_unique_watched(user_data) | ||
|
|
||
| for movie in friends_unique_movies: | ||
| if movie["host"] in user_data["subscriptions"]: | ||
| recommendations.append(movie) | ||
|
|
||
| return recommendations |
| recommendations = [] | ||
| most_watched_genre = get_most_watched_genre(user_data) | ||
| friends_unique_movies = get_friends_unique_watched(user_data) | ||
|
|
||
| for movie in friends_unique_movies: | ||
| if movie["genre"] == most_watched_genre: | ||
| recommendations.append(movie) | ||
|
|
||
| return recommendations |
There was a problem hiding this comment.
Well done on this function! Great use of the previous functions you have written!
|
|
||
| def get_rec_from_favorites(user_data): | ||
| friends_movies = get_friends_movies_list_no_duplicates(user_data["friends"]) | ||
| recommended = get_unique_list_1_movies(user_data["favorites"], friends_movies) |
There was a problem hiding this comment.
I love this use of a helper function you wrote to help you with wave 3 to help you again here! This is well written and really cuts down on what you need to do for the get_rec_from_favorites function!
3 participants
No description provided.