Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ocelots -- Monica B. #6

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

Yf-Monica-Bao
Copy link

No description provided.

Copy link

@kelsey-steven-ada kelsey-steven-ada left a comment

Choose a reason for hiding this comment

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

🎉 Congrats on completing your first project at Ada! 🎉 I’ve added some suggestions & questions, let me know if there's anything I can clarify.

# *******************************************************************************************
# ****** Add assertions here to test that the correct movie was added to "watched" **********
# *******************************************************************************************
assert updated_data["watched"][0] == "It Came from the Stack Trace"

Choose a reason for hiding this comment

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

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.

    assert updated_data["watched"][0] == "It Came from the Stack Trace"
    assert updated_data["watched"][0] == RATING_1
    assert updated_data["watched"][0] == GENRE_1

# *******************************************************************************************
# ****** Add assertions here to test that the correct movie was added to "watched" **********
# *******************************************************************************************
assert updated_data["watched"][1] == "It Came from the Stack Trace"

Choose a reason for hiding this comment

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

Like the test above, we would want to confirm the rest of the data in the movie updated_data["watched"][1] holds the expected values.

The README isn't explicit about what order we should add movies to the list watched. If we want our tests to be independent from a specific implementation that adds movies to a particular location in the watched list, we could use the in keyword to check if the dictionary that the variable movie_to_watch points to is in the watched list instead of looking at a specific index:

assert movie_to_watch in updated_data["watched"]

# Another option:
assert HORROR_1 in updated_data["watched"]

Comment on lines +57 to +59
test = {'genre': 'Fantasy', 'rating': 4.0,
'title': 'The Programmer: An Unexpected Stack Trace'}
assert test in friends_unique_movies

Choose a reason for hiding this comment

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

Nice check for FANTASY_4 👍 I would suggest checking for each of the movies we expect in friends_unique_movies to ensure only what we expected was added to the list.

assert INTRIGUE_3 in friends_unique_movies
assert HORROR_1 in friends_unique_movies
assert FANTASY_4 in friends_unique_movies

Comment on lines +62 to +65
recommendations = get_new_rec_by_genre(sonyas_data)

# Assert
assert len(recommendations) == 0

Choose a reason for hiding this comment

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

Great act & assert steps!

def create_movie(title, genre, rating):
pass
if not title or not genre or not rating:

Choose a reason for hiding this comment

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

Great use of the truthy/falsy values to check for empty strings or None in one statement!

# -----------------------------------------
def get_unique_watched(user_data):
result = []
friends_watched_movies = set()

Choose a reason for hiding this comment

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

Neat use of set to ensure we don't have duplicates in the friends movies!

Comment on lines +81 to +86
friends_watched_movies = []

for friend in user_data["friends"]:
for movie_record in friend["watched"]:
if movie_record not in friends_watched_movies:
friends_watched_movies.append(movie_record)

Choose a reason for hiding this comment

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

Both get_unique_watched and get_friends_unique_watched have similar code that creates a collection representing the movies in user_data["friends"]. Is there a way we could restructure these functions or add a helper function that would let us share code?

# -----------------------------------------
def get_available_recs(user_data):
result = []
friends_watched_movies = get_friends_unique_watched(user_data)

Choose a reason for hiding this comment

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

Nice code reuse!

Comment on lines +100 to +102
for data in friends_watched_movies:
if data["host"] in user_data["subscriptions"]:
result.append(data)

Choose a reason for hiding this comment

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

Nice algorithm! Another way we could approach filtering the unique_watched list is with a list comprehension:

# list comprehension
result = [movie for movie in friends_watched_movies if movie["host"] in user_data["subscriptions"]]

This line is a bit long, in practice we would split a statement like this across lines, use some extra variables, or shorten some naming to keep under the PEP8 guide of 79 characters max per line:

# list comprehension
result = [movie for movie in friends_watched_movies 
         if movie["host"] in user_data["subscriptions"]]

return result


def get_rec_from_favorites(user_data):

Choose a reason for hiding this comment

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

Nice wave 5 implementations!

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