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

Ada C22 Phoenix Class Madina Dzhetegenova #20

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

Conversation

Madina-j
Copy link

No description provided.

Copy link

@anselrognlie anselrognlie left a comment

Choose a reason for hiding this comment

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

Good work! Please review my comments, and let me know if you have any questions. Please feel free to ask me questions directly in Slack, or after you've had a chance to review this together, we could set up a meeting to follow up on anything you have questions about.

@@ -158,13 +158,13 @@ def test_moves_movie_from_watchlist_to_empty_watched():
# Assert
assert len(updated_data["watchlist"]) == 0
assert len(updated_data["watched"]) == 1

raise Exception("Test needs to be completed.")
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.

While it's probably the case that if the title is correct, the rest of the data is as well, we should check all three pieces of data to be sure. We can either do this by checking them individually, or we could make a movie dictionary instance to compare.

@@ -182,13 +182,14 @@ def test_moves_movie_from_watchlist_to_watched():
# Assert
assert len(updated_data["watchlist"]) == 1
assert len(updated_data["watched"]) == 2

raise Exception("Test needs to be completed.")
assert HORROR_1 in updated_data["watched"]

Choose a reason for hiding this comment

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

I like the use of an in check here, since that won't care where in the watched list the movie ends up.

It would probably also be good to ensure that the movie left in the watchlist is the movie we expect, and that the other movie that started in the watched is still there.

Comment on lines +61 to +63
assert INTRIGUE_3 in friends_unique_movies
assert FANTASY_4 in friends_unique_movies
assert HORROR_1 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 way to check tht the three expected movies are there without worrying about at what position each ended up.

Comment on lines +55 to +59
# Act
recommendations = get_new_rec_by_genre(sonyas_data)

# Assert
assert len(recommendations) == 0

Choose a reason for hiding this comment

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

👍 This test needed both the act and the assert.

@@ -1,23 +1,170 @@
import copy

Choose a reason for hiding this comment

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

The copy module provides deepcopy, but we don't need that in this project. In particular, in the function deepcopy is being used, we're only reading the user_data structure. Check out the feedback in get_friends_unique_watched for more.

Comment on lines +146 to +157
def get_rec_from_favorites(user_data):
my_data = user_data["favorites"].copy()

friends_data = user_data["friends"]
for watched in friends_data:
their_watched = watched["watched"]
for their_movie in their_watched:
for i in range(0, len(my_data)):
if their_movie["title"] != my_data[i]["title"]:
del my_data[i]
break
return my_data

Choose a reason for hiding this comment

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

👀 This version of the function is replaced by the later one starting on line 159. Python lets us redefine a function (the "name" of a function is really just a variable which can be overwritten), so whatever implementation comes last will be the one that remains available.

Choose a reason for hiding this comment

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

This approach is very similar to the logic in get_unique_watched except that it's starting from the user favorites rather than the user watched movies. We could remove the logic duplication either by:

  1. Starting with the unique user watched movies and filtering them to those that are also favorites, or
  2. Writing a helper function that filters an arbitrary movie list against the friends data, and use that function for both get_unique_watched and get_rec_from_favorites, passing in either the user watched list or favorite list as needed.

Comment on lines +161 to +162
for movie in user_data["favorites"]:
my_data[movie["title"]] = movie

Choose a reason for hiding this comment

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

👍 This chunk of the code makes an "index" of the user's favorite movies by title.

Comment on lines +164 to +169
friends_data = user_data["friends"]
for watched in friends_data:
their_watched = watched["watched"]
for their_movie in their_watched:
title = their_movie["title"]
my_data.pop(title, None)

Choose a reason for hiding this comment

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

👍 This code iterates through all the friends watched movies, eliminating the ones from the favorite index by title. Popping by key is a constant time operation, so this is pretty efficient! When we're done, only favorite movies not watched by any friends will remain in the index.

for their_movie in their_watched:
title = their_movie["title"]
my_data.pop(title, None)
return my_data.values()

Choose a reason for hiding this comment

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

👍 This returns the movie records for the favorite movies not watched by the friends. Note that this does not return a list. It returns a dictionary value iterator. The prompt asks that we return a list, so this result should be passed to the list function to make a list of the values.

    return list(my_data.values())

Choose a reason for hiding this comment

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

A lot of the logic in this function is dealing with excluding movies the friends have watched. What if we started with a list of movies only the user has watched (we have a helper from a previous wave for this!), and filtered that list to only those in the favorites list?

Comment on lines +122 to +123
if their_movie["host"] not in subscriptions:
continue

Choose a reason for hiding this comment

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

I typically use a continue in a loop more when there is a main chunk of code that can be run without fruther indentation once the continue case has been handled. So like here, I'd tend to write this logic as we have here only if the main logic that follows (the appending steps) could happen without further indentation.

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.

None yet

3 participants