C19 Digital / Amber S #69
Conversation
Co-authored-by: echotangopizza <echotangopizza@users.noreply.github.com>
There was a problem hiding this comment.
Great job with this project! I like the frequent Git commits, good choice of variable names, and the use of helper functions!
I noticed one test was skipped in wave 03. I pulled the repo down and ran the test, which passed 🎉 but an important thing to read back over your PR before submitting to make sure everything looks correct (this is definitely true both here and very much so in industry!).
| # ******************************************************************************************* | ||
| # ****** Add assertions here to test that the correct movie was added to "watched" ********** | ||
| # ******************************************************************************************* | ||
| assert updated_data["watched"][0]["title"] == MOVIE_TITLE_1 |
There was a problem hiding this comment.
Nice job, this is exactly what you want to check!
| # ******************************************************************************************* | ||
| # ****** Add assertions here to test that the correct movie was added to "watched" ********** | ||
| # ******************************************************************************************* | ||
| assert movie_to_watch in updated_data["watched"] |
| @@ -55,7 +55,9 @@ def test_friends_unique_movies_not_duplicated(): | |||
| # Assert | |||
| assert len(friends_unique_movies) == 3 | |||
|
|
|||
| raise Exception("Test needs to be completed.") | |||
| assert INTRIGUE_3 in friends_unique_movies | |||
There was a problem hiding this comment.
Great job with adding these assertions as well!
| # ****** Complete the Act and Assert Portions of these tests ********** | ||
| # ********************************************************************* | ||
| # Assert | ||
| assert len(recommendations) == 0 |
There was a problem hiding this comment.
Nice, this is the same check I would write as well!
| if title and genre and rating: | ||
| movie = {"title": title, "genre": genre, "rating": rating} | ||
| return movie | ||
| else: |
There was a problem hiding this comment.
Since you return from the if clause, there's no need for returning from the else here. If the if block doesn't get executed, the function will return the value from the else clause, so you can omit it and just return from the main body of the function, which will allow you to save on indentation and make for better readability!
| user_unique_watched = [] | ||
|
|
||
| for movie in user_watched: | ||
| if not movie in friends_watched: |
There was a problem hiding this comment.
The logic in this case would work the same way, but it is more "Pythonic" to use the if not X construction.
|
|
||
| return friends_unique_watched | ||
|
|
||
| # def friends_movies(user_data): |
There was a problem hiding this comment.
No problem to leave it in this time, but generally you'll want to remove commented out code before submitting a PR.
| # ----------------------------------------- | ||
| # ------------- WAVE 4 -------------------- | ||
| # ----------------------------------------- | ||
|
|
||
| def get_available_recs(user_data): |
There was a problem hiding this comment.
Great job reusing the get_friends_unique_watched function here!
| # ----------------------------------------- | ||
| # ------------- WAVE 5 -------------------- | ||
| # ----------------------------------------- | ||
|
|
||
| def get_new_rec_by_genre(user_data): |
| if movie in user_data["favorites"]: | ||
| rec_based_on_favorites.append(movie) | ||
| else: | ||
| rec_based_on_favorites = [] |
There was a problem hiding this comment.
No need to use this else here since you probably wouldn't want to reassign the rec_based_on_favorites variable to an empty list. We aren't testing a case that would show this bug, but it would be one like:
test_data = {
"favorites": [INTRIGUE_1, FANTASY_3b],
"watched": [
FANTASY_1b,
],
"friends": [
{
"watched": [
FANTASY_3b,
]
}
]
}I think in this case, INTRIGUE_1 should be returned as a recommendation, correct? This function would return an empty list in this case. Could be something worth investigating!
No description provided.