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

Kunzite - Celina B and Diana S #67

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

celina-barron
Copy link

No description provided.

Copy link

@spitsfire spitsfire left a comment

Choose a reason for hiding this comment

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

Good going, Celina and Diana! I've left suggestions and feedback below in your code.

Some things to consider:

  • Make sure your variables are descriptive so that anyone reading your code will immediately understand what the function or code block is trying to accomplish
  • Use get_most_watched_genre to help complete get_new_rec_by_genre

Comment on lines +6 to +11
movie = {}
if not title or not genre or not rating:
return None
else:
movie.update({"title" : title, "genre" : genre, "rating" : rating})
return movie

Choose a reason for hiding this comment

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

Good job on the guard clause! Let's move it to the top, though. That way we aren't creating data in memory that will be immediately tossed if the function hits lines 7-8.

Let's also bring the main focus of the function (creating a movie and returning it) outside a code block.

    if not title or not genre or not rating:
        return None

    movie = {"title" : title, "genre" : genre, "rating" : rating}
    return movie

Comment on lines +14 to +15
user_data["watched"].append(movie)
return user_data

Choose a reason for hiding this comment

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

Consider validating our inputs first to check that movie and/or user_data is what we think it is:

if movie:
    user_data["watched"].append(movie)

It's small and not necessarily important when passing these tests, but we never want to assume that a user will use our functions correctly.

Comment on lines +18 to +19
user_data["watchlist"].append(movie)
return user_data

Choose a reason for hiding this comment

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

Same as above! Let's check input validity!

return user_data

def watch_movie(user_data, title):
print(user_data)

Choose a reason for hiding this comment

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

Always remember to delete any print statements for debugging purposes when submitting a finale project!

Suggested change
print(user_data)

def watch_movie(user_data, title):
print(user_data)
watchlist_list_of_dict = user_data.get("watchlist")
print(watchlist_list_of_dict)

Choose a reason for hiding this comment

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

Suggested change
print(watchlist_list_of_dict)

Comment on lines +150 to +151
for i in friends_recommendations_list_of_dict:
if i.get("genre") == frequently_watched_genre_list:

Choose a reason for hiding this comment

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

Let's come up with a variable name that's more descriptive than i

Choose a reason for hiding this comment

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

What is frequently_watched_genre_list? Is it the most watched genre type, or is it a list? Can we compare a string to a list? Or, should frequently_watched_genre_list be something else?

What if we call one of our functions to get us the most watched genre type, then use it to compare against our recommendation list?

Comment on lines +191 to +192
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.

👍

# *******************************************************************************************
# ****** Add assertions here to test that the correct movie was added to "watched" **********
# *******************************************************************************************

@pytest.mark.skip()
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.

👍

Comment on lines +62 to +64
assert INTRIGUE_3 in friends_unique_movies
assert HORROR_1 in friends_unique_movies
assert FANTASY_4 in friends_unique_movies

Choose a reason for hiding this comment

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

👍

Comment on lines +62 to +69
highest_genre = None
highest_count_sofar = 0
# loop through most watched dict to update
for genre, count in most_watched_genre_dict.items():
if count > highest_count_sofar:
highest_count_sofar = count
highest_genre = genre
#this better work

Choose a reason for hiding this comment

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

Solely argument's sake, what's another way we might calculate the highest watched genre with our most_watched_genre_dict dictionary and, say, the max function? In what ways might max help make our function more efficient? Or not?

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