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 class - Nadxelle Hernandez #3

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

Conversation

nadxelleHernandez
Copy link

Finished project

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.

Comment on lines +116 to +117
assert type(updated_data["watched"][0]) == dict
assert updated_data["watched"][0]["title"] == MOVIE_TITLE_1

Choose a reason for hiding this comment

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

Nice assertions!

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

Comment on lines +136 to +137
assert movie_to_watch not in updated_data["watchlist"]
assert movie_to_watch in updated_data["watched"]

Choose a reason for hiding this comment

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

Great use of movie_to_watch to confirm the object we set up in the # Arrange step is in the watched list and was removed from watchlist!

Comment on lines +59 to +61
assert FANTASY_4 in friends_unique_movies
assert HORROR_1 in friends_unique_movies
assert INTRIGUE_3 in friends_unique_movies

Choose a reason for hiding this comment

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

Great checks for all the relevant data!

Comment on lines +56 to +58
recomendations = get_new_rec_by_genre(sonyas_data)

assert len (recomendations) == 0

Choose a reason for hiding this comment

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

Nice act & assert steps!

@@ -1,23 +1,194 @@
# ------------- WAVE 1 --------------------

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!

if not user_data:
return None

return get_not_watched_by_friends(user_data["watched"],user_data["friends"])

Choose a reason for hiding this comment

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

This function has 3 statements, and the code in both this function and get_not_watched_by_friends doesn't seem reusable by others. In this situation I would strongly consider if we benefit from a second function.

If we wanted to create a helper function for organization, I might suggest creating one function that performs the first task of creating a list of all of the movies in user_data["friends"]. Then in get_unique_watched we could call that function and use the result to perform the second task of finding movies from user_data["watched"] that are not in our list of friend movies. This organization gives us 2 functions that each have a single purpose, and we could reuse the function to get the list of friend movies in get_friends_unique_watched below.

friends_watched = get_friends_unique_watched(user_data)

for movie in friends_watched:
if movie["host"] in user_data["subscriptions"] and movie not in recomendations:

Choose a reason for hiding this comment

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

The PEP 8 style guide recommends limiting lines to a max of 79 characters to make code easier to read, especially with multiple editor windows open: https://peps.python.org/pep-0008/#maximum-line-length. We would want to break it over a couple lines or create variables to hold the comparisons that we then use in an if statement:

is_host_in_subscriptions = movie["host"] in user_data["subscriptions"]
is_movie_in_recommendations = movie in recomendations
if is_host_in_subscriptions and not is_movie_in_recommendations:
    recomendations.append(movie)

return None

recomendations = list()
friends_watched = 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.

Great code reuse!

friends_watched = get_friends_unique_watched(user_data)

for movie in friends_watched:
if movie["host"] in user_data["subscriptions"] and movie not in recomendations:

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 
         if movie["host"] in user_data["subscriptions"]]

if not user_data:
return None

return get_not_watched_by_friends(user_data["favorites"],user_data["friends"])

Choose a reason for hiding this comment

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

What a cool way to reuse get_not_watched_by_friends 😄

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.

2 participants