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

Panthers - Cintia Shamsu #29

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

cintiashamsu
Copy link

No description provided.

Copy link
Contributor

@audreyandoy audreyandoy 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 Cintia! You hit all the learning goals for this project and all tests are passing. Your code was very clean and well-organized. You have earned a well-deserved 🟢 grade on this project 🐈‍⬛ ✨

I added comments to your PR primarily on ways to add list comprehension to refactor your code. In the future, I'd like to see you use more guard clauses and docstrings in your functions. Docstrings are a great way of leaving notes to your future self about your approach to writing these functions (especially about how mean and mode work).

Feel free to refactor and improve on this project and do let me know if you want me to take another look for fun 😄

Comment on lines +121 to +124
assert updated_data["watched"][0]["title"] == MOVIE_TITLE_1
assert updated_data["watched"][0]["genre"] == GENRE_1
assert updated_data["watched"][0]["rating"] == RATING_1
assert updated_data["watchlist"] == []
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Nice work! I would remove the last test as the first test of checking if the watchlist is empty is sufficient enough.

Comment on lines +144 to +145
assert updated_data["watched"][1] == movie_to_watch
assert updated_data["watched"][0] == FANTASY_2
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Comment on lines +57 to +59
assert INTRIGUE_3 in friends_unique_movies
assert FANTASY_4 in friends_unique_movies
assert HORROR_1 in friends_unique_movies
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Comment on lines +54 to +58
# Act
result = get_new_rec_by_genre(sonyas_data)

# Assert
assert len(result) == 0
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Comment on lines +7 to +11
if title == None or genre == None or rating == None:
return None
else:
movie = {"title" : title, "genre" : genre, "rating" : rating}
return movie
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if title, genre, or rating were entered as empty strings, 0, or False? We can make your conditional more robust by checking for all falsey values instead. The else clause is also optional here and we can omit it, allowing the creation of the movie dictionary to shine and be more in focus.

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

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

Comment on lines +71 to +76
def get_user_data(user_data):
self = []
for data in user_data["watched"]:
if data not in self:
self.append(data)
return self
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Nice work preventing duplicate movies from being added to the list.

Comment on lines +79 to +86
def get_unique_watched(user_data):
unique = []
friends = get_friend_data(user_data)
user = get_user_data(user_data)
for movie in user:
if movie not in friends:
unique.append(movie)
return unique
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Nice helper functions!

Comment on lines +89 to +96
def get_friends_unique_watched(user_data):
unique = []
friends = get_friend_data(user_data)
user = get_user_data(user_data)
for movie in friends:
if movie not in user:
unique.append(movie)
return unique
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Nice helper functions, again!

Comment on lines +103 to +111
def get_available_recs(user_data):
recs = []
friends = get_friend_data(user_data)
user = get_user_data(user_data)
for movie in friends:
if movie not in user:
if movie["host"] in user_data["subscriptions"]:
recs.append(movie)
return recs
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Nice work! We can use combined conditional expressions to combine the two if statements:

def get_available_recs(user_data):
  recs = []
  friends = get_friend_data(user_data)
  user = get_user_data(user_data)
  for movie in friends:
    if movie not in user and movie["host"] in user_data["subscriptions"]:
        recs.append(movie)
  return recs

Comment on lines +118 to +136
def get_new_rec_by_genre(user_data):
recs = []
user = get_user_data(user_data)
friends = get_friend_data(user_data)
fave = get_most_watched_genre(user_data)
for movie in friends:
if movie not in user and movie["genre"] == fave:
recs.append(movie)
return recs

# Recommends movies from user's fav list that no friends have watched
def get_rec_from_favorites(user_data):
recs = []
friends = get_friend_data(user_data)
fave = user_data["favorites"]
for each in fave:
if each not in friends:
recs.append(each)
return recs
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Great work utilizing helper functions in both of these functions. How would we use list comprehensions to clean up our code?

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