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

Otters Class - Luz Andrea Garcia Zapata. #99

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

Conversation

andrygzt
Copy link

No description provided.

Copy link

@jbieniosek jbieniosek left a comment

Choose a reason for hiding this comment

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

Great work on this project Andrea! I like your set and dictionary based solutions. I have some notes on ways that you can DRY (Don't Repeat Yourself) your code to improve readability. This project is green.

# ------------- WAVE 1 --------------------

def create_movie(title, genre, rating):
pass
new_movie = {}
if title and genre and rating:

Choose a reason for hiding this comment

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

👍

if user_data["watchlist"][num_movies]["title"] == title:
movie = user_data["watchlist"].pop(num_movies)
user_data["watched"].append(movie)
break

Choose a reason for hiding this comment

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

Fantastic! Modifying a list as it's being iterated over is an "anti-pattern" that can cause a lot of bugs and errors. Breaking out of the list at this point ends the loop and eliminates the possibility for those bugs. Replacing the break statement with the return statement on line 28 would also work here.


# -----------------------------------------
# ------------- WAVE 2 --------------------
# -----------------------------------------

def get_watched_avg_rating(user_data):
addition = 0

Choose a reason for hiding this comment

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

For readability, I recommend variable names that describe what is being stored in the variable - in this case, something like sum_ratings or ratings_total.

Comment on lines +56 to +61
popular_genre = None
counter_genre = 0
for genre in genre_dict:
if genre_dict[genre] > counter_genre:
popular_genre = genre
counter_genre = genre_dict[genre]

Choose a reason for hiding this comment

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

💯

movie_dict[movie["title"]] = movie
friend_set.add(movie["title"])

unique_set = user_set - friend_set

Choose a reason for hiding this comment

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

Neat solution!

if movie_dict[movies]["host"] in user_data["subscriptions"]: #Compare if movies that friends has watched and recommend are hosted in services is is subscripted
unique_list.append(movie_dict[movies])

return unique_list

Choose a reason for hiding this comment

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

This function is not solving the correct problem. This function is compiling a list of the the movies that the friends have watched the that user has not, and then checking to see if the genre of the movies in that list shows up in the "favorites" list. However, the prompt is asking for recommendations of the user's most frequently watched genre. To correctly solve this problem, this function would need to first get the most frequently watched genre (ie, get_most_watched_genre()), then check each movie in the friends_unique_set against that. The reason why this function passes all of the tests is because the the user_data["subscriptions"] test happens to weed out the incorrect data.


user_unique_set = user_set - friend_set #change order to hold the difference from favorites.

if "favorites" in user_data: #Make sure this key exist in the dictionary

Choose a reason for hiding this comment

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

This check will always pass, because the key is accessed on line 218. If the user_data does not contain the key "favorites", line 218 will throw a KeyError, and the function will end before this line.

@@ -145,8 +145,8 @@ def test_moves_movie_from_watchlist_to_watched():
# *******************************************************************************************
# ****** Add assertions here to test that the correct movie was added to "watched" **********
# *******************************************************************************************
assert updated_data["watched"][1] is HORROR_1
Copy link

@jbieniosek jbieniosek Mar 29, 2022

Choose a reason for hiding this comment

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

In this case I suggest assert HORROR_1 in updated_data["watched"]. The requirements don't specify in what order the movie should be added to the "watched" list.

Comment on lines +124 to +125
assert updated_data["watched"][0]["rating"] is RATING_1
assert updated_data["watched"][0]["genre"] is GENRE_1

Choose a reason for hiding this comment

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

In this case where the code is checking individual keys of the dictionary, I recommend including all of the relevant keys. Also, this was an error on our side, but "is" can cause some issues and "==" is the preferred way to check equality.

Suggested change
assert updated_data["watched"][0]["rating"] is RATING_1
assert updated_data["watched"][0]["genre"] is GENRE_1
assert updated_data["watched"][0]["rating"] == RATING_1
assert updated_data["watched"][0]["genre"] == GENRE_1
assert updated_data["watched"][0]["title] == MOVIE_TITLE_1

@@ -58,8 +58,10 @@ def test_friends_unique_movies_not_duplicated():
# *************************************************************************************************
# ****** Add assertions here to test that the correct movies are in friends_unique_movies **********
# **************************************************************************************************
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.

👍

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