Snow leopards - Jessica Hebert C18 #165
Conversation
There was a problem hiding this comment.
Great work on your first Unit 1 project, Jessica! 🟢 for Viewing Party!
I left a couple of comments about how to make your code more Pythonic and also called out a coding pattern that you can easily implement to make your code even more readable. Let me know if you have any questions!
| @@ -118,13 +118,13 @@ def test_moves_movie_from_watchlist_to_empty_watched(): | |||
| # Assert | |||
| assert len(updated_data["watchlist"]) == 0 | |||
| assert len(updated_data["watched"]) == 1 | |||
|
|
|||
| raise Exception("Test needs to be completed.") | |||
| assert updated_data["watched"][0]["title"] is MOVIE_TITLE_1 | |||
| # raise Exception("Test needs to be completed.") | ||
| # ************************************************************************************************* | ||
| # ****** Add assertions here to test that the correct movies are in friends_unique_movies ********** | ||
| # ************************************************************************************************** | ||
|
|
There was a problem hiding this comment.
Oop, looks like you commented out the Exception, but didn't add more assertions to this test. What assertions can you write to test that there are no duplicated movies in the list of movies watched only by friends?
| new_move = {"title": title, | ||
| "genre" : genre, | ||
| "rating": rating} | ||
| for key, value in new_move.items(): | ||
| if value == None: | ||
| return None | ||
| else: | ||
| return new_move |
There was a problem hiding this comment.
Nice work here! A couple of call outs:
Instead of first using the data to create a dictionary and then iterating over the keys/values to check if any of the values are none, we should first check if any of the arguments are invalid and immediately return None.
Then we can avoid creating a new_movie and skip iterating.
To do so in a Pythonic way, you can write:
if not title and not genre and rating:
return None
# then you can return a dictionary literal
return { "title": title,
"genre" : genre,
"rating": rating
}When you have an if statement that checks if the arguments are invalid and then return None, we're using the guard clause pattern. The Guard Clause pattern helps in these cases by “inverting” that way of thinking and rapidly returning false conditions and keeping the important code blocks (usually the longest) in the lowest indentation level possible. Read more here
This way, when another developer comes along, they can immediately recognize "Oh if these values aren't valid, then we'll return None."
| user_data = {"watched":[movie]} | ||
| return user_data |
There was a problem hiding this comment.
When you inspect user_data (as it is passed in from the test, we can see that user_data is already a dict with a key called "watched" (see lines 69-71 of test_wave_01.py:
user_data = {
"watched": []
}Since that's the case, when you reassign user_data to a dictionary literal on line 17 then you overwrite anything that was previously in the dictionary ("friends", "watchlist"). In this example, we have a pretty bare bones user_data so the test does pass, but if user_data had other key/value pairs, we don't want to overwrite them and lose the data.
Since "watched" is already list, you don't need to put movie into a list with [movie].
We can just write:
def add_to_watched(user_data, movie):
user_data{"watched"].append(movie)
return user_data| def add_to_watchlist(user_data,movie): | ||
| user_data = {"watchlist":[movie]} | ||
| return user_data |
There was a problem hiding this comment.
Same comment as above, we want to avoid completely overwriting user_data.
This function can use the append method to add movie to the watchlist
| # ----------------------------------------- | ||
| # ------------- WAVE 4 -------------------- | ||
| # ----------------------------------------- | ||
| def get_available_recs(user_data): | ||
| movies_not_watched= get_friends_unique_watched(user_data) |
There was a problem hiding this comment.
Nice work reusing the method you already wrote
| def get_available_recs(user_data): | ||
| movies_not_watched= get_friends_unique_watched(user_data) | ||
| recommended_movies = [] | ||
| subscription =user_data["subscriptions"] |
There was a problem hiding this comment.
For consistent styling that conforms to the Python style guide, we should have a space on each side of the equal sign
| # ----------------------------------------- | ||
| # ------------- WAVE 4 -------------------- | ||
| # ----------------------------------------- | ||
| def get_available_recs(user_data): | ||
| movies_not_watched= get_friends_unique_watched(user_data) |
There was a problem hiding this comment.
Consider renaming movies_not_watched to something more descriptive, like friends_watched_movies
|
|
||
| # ----------------------------------------- | ||
| # ------------- WAVE 5 -------------------- | ||
| # ----------------------------------------- | ||
| def get_new_rec_by_genre(user_data): | ||
| recommended_movies =[] | ||
| genre =get_most_watched_genre(user_data) |
| genre =get_most_watched_genre(user_data) | ||
|
|
||
| for movie_not_watched in get_friends_unique_watched(user_data): | ||
| if movie_not_watched["genre"] is genre: |
No description provided.