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

Sea_Turtles_Ashley_Benson #116

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

Conversation

bensonaa1988
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.

Nice job!

All your tests are passing, and your code is nicely structured, and makes good re-use of functions from earlier waves. Nice custom helper function as well!

We'll start looking more at performance and efficiency going ahead, so I included some thoughts there as well.

Make sure to review the tests. There were a couple that had instructions about things to add that it looks like were missed. So while getting those green checks in VSCode feels great, make sure to review their implementation as there will occasionally be parts to fill in.

Well done!

Comment on lines 78 to 80
assert updated_data["watched"][0]["title"] is MOVIE_TITLE_1
assert updated_data["watched"][0]["genre"] is GENRE_1
assert updated_data["watched"][0]["rating"] is RATING_1

Choose a reason for hiding this comment

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

It was our bad shipping with these is comparisons. For this project, the is string checks should be ok, but in general, is is mostly used to check for None. Its purpose is to check whether two objects are the same object, while == checks whether the values of two objects are the same.

@@ -123,7 +123,7 @@ def test_moves_movie_from_watchlist_to_empty_watched():
# ****** Add assertions here to test that the correct movie was added to "watched" **********

Choose a reason for hiding this comment

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

Note that this test is incomplete. It checks a few assertions, but should also check that the movie in the moved list is as expected.

@@ -146,7 +146,7 @@ def test_moves_movie_from_watchlist_to_watched():
# ****** Add assertions here to test that the correct movie was added to "watched" **********

Choose a reason for hiding this comment

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

Note that this test is incomplete. It checks a few assertions, but should also check that the new movie in the moved list is as expected.

@@ -59,7 +59,7 @@ def test_friends_unique_movies_not_duplicated():
# ****** Add assertions here to test that the correct movies are in friends_unique_movies **********

Choose a reason for hiding this comment

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

Note that this test is incomplete. It checks a few assertions, but should also check for the correct movies in the result.

@@ -57,7 +57,7 @@ def test_new_genre_rec_from_empty_friends():
# ****** Complete the Act and Assert Portions of theis tests **********

Choose a reason for hiding this comment

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

Note that this test is incomplete. It only defines some data, but has no act or assert. Pytest assumes that any test that raises no errors is a passing test, which is why this "passed." But it's not actually doing anything.

# -----------------------------------------
# ------------- WAVE 4 --------------------
# -----------------------------------------

def get_available_recs(user_data):

Choose a reason for hiding this comment

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

👍

def get_available_recs(user_data):
users_data = user_data
recommended_movies = []
friends_unique_watched = get_friends_unique_watched(user_data)

Choose a reason for hiding this comment

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

Nice reuse of functions from wave 3!

# -----------------------------------------
# ------------- WAVE 5 --------------------
# -----------------------------------------

def get_new_rec_by_genre(user_data):

Choose a reason for hiding this comment

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

👍

Double function reuse!

Comment on lines +155 to +180
def get_new_rec_by_genre(user_data):

users_data = user_data
new_rec_by_genre = []
most_watched_genre = get_most_watched_genre(users_data)
friends_unique_watched = get_friends_unique_watched(users_data)

for movie in friends_unique_watched:
if movie["genre"] == most_watched_genre:
new_rec_by_genre.append(movie)

return new_rec_by_genre


def get_rec_from_favorites(user_data):

users_data = user_data
favorites = users_data["favorites"]
recommend = []
friends_watched_movies = friends_watched(users_data)

for movie in favorites:
if movie not in friends_watched_movies:
recommend.append(movie)
return recommend

Choose a reason for hiding this comment

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

It looks like your wave 5 functions got duplicated here.

Comment on lines +203 to +208
friends_watched_movies = friends_watched(users_data)

for movie in favorites:
if movie not in friends_watched_movies:
recommend.append(movie)
return recommend

Choose a reason for hiding this comment

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

This approach starts from the favorite movies list, and only recommends it if it's not in the list of movies watched by the friends. An alternative approach would be to start from the unique user's watched movies, and only recommend a movie it it appears in the user's favorite list. And for either approach, we could use title sets to improve the performance of lookups (compared to in on a list).

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