Skip to content

Comments

Cedar - Lizet Tovar#98

Open
lizett wants to merge 2 commits intoAda-C16:masterfrom
lizett:master
Open

Cedar - Lizet Tovar#98
lizett wants to merge 2 commits intoAda-C16:masterfrom
lizett:master

Conversation

@lizett
Copy link

@lizett lizett commented Sep 20, 2021

No description provided.

Copy link

@kaidamasaki kaidamasaki left a comment

Choose a reason for hiding this comment

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

Great job!

It looks like you just ran out of time to finish debugging a few things. Overall your code looks pretty solid though!

I did point out a few style things but they're really minor.

Well done!

Comment on lines +4 to +7
"""
This function takes title, genre and ratings as parameters and returns
the input arguments as values in a dictionary.
"""

Choose a reason for hiding this comment

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

Nice use of docstrings. 😃

Comment on lines +8 to +13
new_movie = {}
if title and genre and rating:
new_movie["title"] = title
new_movie["genre"] = genre
new_movie["rating"] = rating
return new_movie

Choose a reason for hiding this comment

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

I recommend creating dictionaries in place when possible:

Suggested change
new_movie = {}
if title and genre and rating:
new_movie["title"] = title
new_movie["genre"] = genre
new_movie["rating"] = rating
return new_movie
if title and genre and rating:
return {
"title": title,
"genre": genre,
"rating": rating
}

for movie in most_watched:
genres.append(movie["genre"])

return mode(genres)

Choose a reason for hiding this comment

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

Nice use of mode. 😃

Comment on lines +141 to +142
if movie["title"] not in user_watched and movie["host"] in user_data["subscriptions"]:
recommended_movies.append(movie)

Choose a reason for hiding this comment

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

You need to also check if the movie is in recommended_movies to avoid inserting the same movie twice.

Comment on lines +168 to +170
if movie not in movies_watched_by_user:
if movie not in friends_watched_movies:
if movie["genre"] in get_most_watched_genre(user_data):

Choose a reason for hiding this comment

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

I'd recommend condensing these into a single compound conditional to avoid so much indentation:

Suggested change
if movie not in movies_watched_by_user:
if movie not in friends_watched_movies:
if movie["genre"] in get_most_watched_genre(user_data):
if movie not in movies_watched_by_user \
and movie not in friends_watched_movies \
and get_most_watched_genre(user_data) # prevent TypeError \
and movie["genre"] in get_most_watched_genre(user_data) \
and movie not in recommended_movies:

This reads more clearly if you're used to reading Python code.

You also have a bug if get_most_watched_genre returns None.

This is where it makes sense to see if the movie is already in recommended_movies.

Choose a reason for hiding this comment

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

And, yeah. These conditions are pretty ugly so breaking things out into different passes of the loop might read more cleanly.

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