Skip to content

Phoenix-Jenny Massari-Anh Tran#16

Open
momofAnAl wants to merge 34 commits intoAda-C22:mainfrom
momofAnAl:main
Open

Phoenix-Jenny Massari-Anh Tran#16
momofAnAl wants to merge 34 commits intoAda-C22:mainfrom
momofAnAl:main

Conversation

@momofAnAl
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown

@apradoada apradoada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 GREEN! 🟢

Well done, y'all! Your code passes all the tests and looks good! I've left some suggestions, but nothing major that requires a second pass! That being said, you are more than welcome to make changes if you so desire! I think overall, the big things I saw were just finding a balance of the docstrings and figuring out what assumptions can be made to cut down on certain code! Other than that it looks great! Well done!

Comment thread tests/test_wave_01.py
Comment on lines +166 to +171
assert updated_data["watched"] == [{
"title": MOVIE_TITLE_1,
"genre": GENRE_1,
"rating": RATING_1
}]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this test! One small change I would suggest might be to make the left side a little more specific! If we're testing to see if an entire list looks like what we're expecting, this is ok, but what we're most curious about is if the movie has been added. I would suggest checking something like:

  assert updated_data["watched"][0] == {
             "title": MOVIE_TITLE_1,
             "genre": GENRE_1,
             "rating": RATING_1
         }

Comment thread tests/test_wave_01.py
# *******************************************************************************************
# ****** Add assertions here to test that the correct movie was added to "watched" **********
# *******************************************************************************************
assert updated_data["watched"] == [FANTASY_2, HORROR_1]
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like what y'all have done here! Y'all know exactly what the "watched" list should look like after the movie gets moved!

Comment thread tests/test_wave_03.py
# **************************************************************************************************

@pytest.mark.skip()
assert friends_unique_movies == [FANTASY_4, HORROR_1, INTRIGUE_3]
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like the other assert you created, I love that you leveraged the fact that you know exactly what the list should look like!

Comment thread tests/test_wave_05.py
empty_list = get_new_rec_by_genre(sonyas_data)

# Assert
assert empty_list == []
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This act and assert are exactly what we were looking for!

Comment thread viewing_party/party.py
"""

if not title or not genre or rating is None:
return None
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great!

Comment thread viewing_party/party.py
if movie not in user_watched and movie not in unique_friends_watched:
unique_friends_watched.append(movie)

return unique_friends_watched
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good! I love that you used your helper function in multiple functions! Really really well done!

Comment thread viewing_party/party.py
Comment on lines +155 to +158
# using max() to get the max count in genere_count dictionary
# most_frequent_genre = max(genres_count, key=genres_count.get)
# If use max(dictionary), max() looks only at the keys of the dictionary, not the values.
# If the keys are strings, max() will return the key that comes last alphabetically
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like it was a comment meant for the developers eyes only! Don't forget to take them out!

Comment thread viewing_party/party.py

recommended_movies = []
for movie in friends_watched:
if movie not in user_watched and movie["host"] in user_data["subscriptions"] \
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember that get_friends_watched is going to hold all the movies your friends have seen and you haven't! Therefore, we don't actually need to check and make sure there is no overlap. Also, because recommended_movies starts empty, we have other checks that make sure duplicates aren't added. As a result, the first and last conditionals aren't necessary!

Comment thread viewing_party/party.py
for movie in friends_watched:
if movie not in user_watched and \
movie["genre"] == most_frequenty_genre and\
movie not in recommended_movies:
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same idea here as before. Every other collection creation should check for duplicates, which means this conditional should be unnecessary!

Comment thread viewing_party/party.py
Returns:
recomemded movies (list)
"""
user_watched = user_data["watched"]
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only use user_watched one other time in this function! When we use a value (`user_data["watched"]') just a few times, it can be a good idea to just avoid using a variable as a middle man. Direct access will work just fine!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants