Viewing party project completion: #23
Conversation
There was a problem hiding this comment.
I've added comments about the implementation as submitted. Please review them, and let me know if you have any questions. There are a few things I'd like you to revisit before we move on from this project. My Slack communication has more detail about exactly what to focus on.
tests/test_wave_01.py
Outdated
| 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.
👍 We should check all three parts of the movie dict.
Here, we are certain that the movie is in position 0 of the watched list (since there's only a single movie). But since the project doesn't specify where in the watched list the movie should appear, we might consider performing an in check, with a movie dict that we construct with the expected values, in cases where other data might be present.
There was a problem hiding this comment.
Updated this in the code and ran tests to ensure everything passes ok.
tests/test_wave_01.py
Outdated
| 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.
👍 We should check all three parts again, and while your implementation does append to the watched list, since we don't have that specified, we might stick with an in check so that doesn't become an assumption in the test (which could restrict our options to refactor later). Here, we also have a variable the refers to the movie that should have moved, which we can reuse.
assert movie_to_watch in updated_data["watched"]Additionally, it would be a good idea to check that the movie remaining in the watchlist and the movie that was already watched are still in the lists we expect to find them in.
| assert INTRIGUE_3 in friends_unique_movies | ||
| assert HORROR_1 in friends_unique_movies | ||
| assert FANTASY_4 in friends_unique_movies |
There was a problem hiding this comment.
👍 Great way to check that the three movies are in the output.
Here, it's even more important not to assume a particular ordering of the outputs. The act of making some data unique can often result in data ending up in arbitrary locations relative to where they occurred in the original data, especially if intermediate data structures like sets or dicts are involved.
tests/test_wave_05.py
Outdated
| #Act | ||
| reccomendations = get_new_rec_by_genre(sonyas_data) | ||
|
|
||
| @pytest.mark.skip() | ||
| #Assert | ||
| assert len(reccomendations) == 0 |
There was a problem hiding this comment.
👍 This test needed both the act and the assert.
There was a problem hiding this comment.
Nit: align the comments to the code and include a space after the #
# Act
reccomendations = get_new_rec_by_genre(sonyas_data)
# Assert
assert len(reccomendations) == 0
viewing_party/__init__.py
Outdated
There was a problem hiding this comment.
👀 No need to include a blank line in this file. Be sure to use git status to make sure we're not staging/committing unintentional changes.
git status shows the commands to use to move a file from staged to unstaged, and unstaged to reverting the change.
viewing_party/party.py
Outdated
| joined_friends_list = join_friends_list(user_data) | ||
| if user_data["watched"] == [] or joined_friends_list == []: | ||
| return new_rec_by_genre |
There was a problem hiding this comment.
While it may seem like a point of efficiency to check whether there are any movies in the user or friends list, and quit early, keep in mind that when there is friend movie data, this results in building a merged list of the movies (which could be large) and is then discarded without further use.
I do see that without this check, one of the tests fails while trying to access the subscription data of the user, but this is because this function was not specified to take subscription/host information into account when making a genre recommendation. In actuality, the first test (the only test of get_new_rec_by_genre that returns a non-empty result) is passing due to a fluke in the data, that the host/subscription data in the less pared down structure does have host and subscription data (it was based on the data from wave 4), and that data happens to include the expected genre recommendation result in the get_available_recs result. This is an error in implementing the specification.
We do have a previous function which, if dropped in in place of the call to get_available_recs does match the specified behavior, and also passes the tests without this initial check.
viewing_party/party.py
Outdated
|
|
||
|
|
||
| most_watched_genre = get_most_watched_genre(user_data) | ||
| unreviewed_recs = get_available_recs(user_data) |
There was a problem hiding this comment.
👀 Using get_available_recs here conflicts with the specified wave behavior. get_available_recs applies host/subscription data when looking for movies watched by the friends, but not by the user, but get_new_rec_by_genre is specified to consider a movie to be part of the result if:
- The user has not watched it
- At least one of the user's friends has watched
- The
"genre"of the movie is the same as the user's most frequent genre
There is no requirement about host/subscription data.
viewing_party/party.py
Outdated
| for movie in favorites: | ||
| if movie not in joined_friends_list: | ||
| rec_from_favorites_list.append(movie) |
There was a problem hiding this comment.
👍 Nice logic to filter the favorites list of movies against the list of movies watched by the friends.
Notice that this logic duplicates logic we've previously implemented, filtering against the movies watched by friends. What if we started with the list of movies watched by the user, but not by the friends (it would be reasonable to assume that a user wouldn't have a movie as a favorite if they haven't watched it)? We already have logic that performs this part of the calculation. Then we could just filter that list against movies in the favorite list.
As with other similar logic elsewhere, it would be worth considering using an auxilliary set of titles to help reduce the cost of checking whether a particular movie is in a certain list.
viewing_party/party.py
Outdated
|
|
||
| def get_rec_from_favorites(user_data): | ||
| favorites = user_data["favorites"] | ||
| joined_friends_list = join_friends_list(user_data) |
There was a problem hiding this comment.
👍 Nice reuse of your joined friend movie helper.
But if we change our perspective a little bit, we might be able to reuse a different function that does some of the same logic also performed later in this function. See my note below.
viewing_party/party.py
Outdated
| if watched_movies == []: | ||
| return None |
There was a problem hiding this comment.
Do we need the dedicated guard clause here? If there were no watched movies, what would your logic produce for a result?
removed join_friends helper function: helper function was adding unnecessary space complexity. updated functions that called on joined friends function to work without helper function. updated wave 1 assert statements to not includee list location and use in method instead. removed erraneous space in __init___ file. adjusted formatting in test wave 5 Made sure to remove redudency by calling previouusly created functions where neccessary. (ex: get_friends_unique()). updated get_new_rec_by_genre result to not be dependent on subscriptions by changing the program to look at get_friends_unique_watched rather than get_available_recs Removed erraneous guard claus that checked if list was empty "
|
@anselrognlie I have updated the code and I think I was able to address the points you mentioned in your pull request! Apologies if I missed anything. |
|
Just committed a new change to update the validation function at the beginning of the code. |
Apologies on the delay. I was unable to really allocate the right time/ brain power to focus on part 3+ of the project till today. I am looking forward to doing some more pair programming in the future to continue building up those skill sets and am bummed I couldn't work with a partner for this project but completely understand the reason why! Once I had a full nights rest I was able to wrap my head around the complexities of the project.
I made some adjustments to my code once I have all tests pass. One was creating a helper function that compiles the movies in friends lists into one list to help streamline future functions. It helped prevent risk of typing in a bug by making the movie compilation a function call instead of having to write out the iterations each time.
now that I have completed I will be sure to watch the viewing party recap and will update absent @ ada with my recap of the things I learned/would do differently.