Sphinx- Tami Gaertner and Tatiana Trofimova - #7
Conversation
kelsey-steven-ada
left a comment
kelsey-steven-ada
left a comment
There was a problem hiding this comment.
Nice work on your first pair project Tami & Tatiana!
| # ******************************************************************************************* | ||
| # ****** Add assertions here to test that the correct movie was added to "watched" ********** | ||
| # ******************************************************************************************* | ||
| assert movie_to_watch in updated_data["watched"] |
There was a problem hiding this comment.
Great use of movie_to_watch to confirm the object we set up in the # Arrange step is in the watched list. How could we also confirm that the remaining movies are still in the lists that we expect?
| # ******************************************************************************************* | ||
| # ****** 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 assertion for the title! In this case where the test is checking individual keys of a dictionary, I recommend including assertions for all of the relevant keys.
watched_movie = updated_data["watched"][0]
assert watched_movie["title”] == MOVIE_TITLE_1
assert watched_movie["rating"] == RATING_1
assert watched_movie["genre"] == GENRE_1| assert len(friends_unique_movies) == 3 | ||
|
|
||
| raise Exception("Test needs to be completed.") | ||
| # raise Exception("Test needs to be completed.") |
There was a problem hiding this comment.
I would like y'all to revisit this test and add assertions that would confirm if the 3 movies we expect are indeed the ones present in friends_unique_movies.
| # ********************************************************************* | ||
| # ****** Complete the Act and Assert Portions of these tests ********** | ||
| # ********************************************************************* | ||
| assert get_new_rec_by_genre(sonyas_data) == [] |
There was a problem hiding this comment.
Nice act and assert, all rolled into one!
Doing both in one line is all good for a test like this. If our Act step or Assert step is more involved (for example, we need to call more than a single function, we have more that 1-2 arguments to the function we're testing, or we need to assert more than a single condition) we'd want to separate out the Act from the Assert step to make the code easier to understand at a glance.
| # Verify the arguments exist | ||
| if title and genre and rating: | ||
| return { | ||
| "title": title, | ||
| "genre": genre, | ||
| "rating": rating, | ||
| } |
There was a problem hiding this comment.
Great use of Truthy/Falsy values to check if the parameters are empty or None!
I would tend to reverse the sense of this comparison to check if NOT each parameter exists. Then we could treat the if-statement as a guard clause (returning None), and unindent the main logic. This makes the intended flow of the function as un-indented and prominent as possible, and makes it explicit what will be returned in either case.
if not title or not genre or not rating::
return None
return {
"title": title,
"genre": genre,
"rating": rating,
}| recommended_title = [] | ||
|
|
||
| for movie in rec_pool: | ||
| if movie["host"] in user_data["subscriptions"]: |
There was a problem hiding this comment.
Extra space between in and the next part of the statement
| if movie["host"] in user_data["subscriptions"]: | |
| if movie["host"] in user_data["subscriptions"]: |
| rec_pool = get_friends_unique_watched(user_data) | ||
|
|
||
| recommended_title = [] | ||
|
|
||
| for movie in rec_pool: | ||
| if movie["host"] in user_data["subscriptions"]: | ||
| recommended_title.append(movie) | ||
| return recommended_title |
There was a problem hiding this comment.
Nice algorithm and code reuse with get_friends_unique_watched!
Some other ways to approach filtering the rec_pool list could be using a list comprehension or using the filter function with a lambda function:
# list comprehension
recommended_title = [movie for movie in rec_pool if movie["host"] in user_data["subscriptions"]]
# filter with a lambda function
recommended_title = filter(lambda movie: movie["host"] in user_data["subscriptions"], rec_pool)(These lines are a bit long, in practice we might split a statement like these across lines, use some extra variables, or shorten some naming to keep under the PEP8 guide of 79 characters max per line.)
More info on Lambda functions in Python in case it’s useful: https://realpython.com/python-lambda/
| """ | ||
| rec_pool = get_friends_unique_watched(user_data) | ||
|
|
||
| recommended_title = [] |
There was a problem hiding this comment.
Since we are returning a list with potentially multiple movies, and we are returning dictionaries of movie info rather than their titles, I would strongly suggest renaming this with a plural name.
What name could better represent that this variable holds info for multiple recommended movies?
| rec_pool = get_friends_unique_watched(user_data) | ||
|
|
||
| fav_genre = get_most_watched_genre(user_data) |
| """ | ||
| Returns a list of user's favorites movies their friends haven't watched. | ||
| """ |
There was a problem hiding this comment.
Since we need to consider movies that the user has watched but their friends have not, how could we reuse get_unique_watched to simplify this function?
3 participants
No description provided.