Sphinx - Vlada Rapaport & Brianna Root

[vladarap88]

No description provided.

[yangashley]

Nice work on viewing-party, Brianna and Vlada!

I also see that you two made frequent commits, nice job! In the future, as you continue to make small, frequent commits, I'd encourage you to practice writing concise and descriptive messages so when someone reviews the commit history there is a clear story of what happened over time.

Let me know if you have any questions about my review comments.

[yangashley]

Indentation was changed here and on lines 156 - 160. The code should be indented 4 spaces in, but is indented 8 spaces. Be mindful of what changes get added to your commit since we don't want to introduce inconsistencies in a project.

[yangashley]

If the order of these movies were somehow changed, then the zero-th element might not be equal to MOVIE_TITLE_1. Do we need to specifically test that the first element of the watched list is equal to that title?

A more general way to write this assertion could look like:

assert HORROR_1 in updated_data["watched"]

Line 163-164 don't really affect the test, but they also aren't testing what we're interested in testing. Per the title of this test, we're looking to test that a movie moves from the watchlist to the watched list. Consider removing these two lines to make the test more concise.

[yangashley]

Note that we don't need to write an assertion to test that watchlist is empty because the movie was moved to the watched list because the assertion was already provided on line 160. Line 165 could be removed to keep the test concise.

Also, since you wrote an assertion to check that HORROR_1 was in watched, you could test the opposite to make that it is not in watchlist, like:

assert HORROR_1 not in updated_data["watchlist"]

[yangashley]

Similar to my comment above, we might consider removing lines 186-187 since this test is not concerned with genre/ratings.

[yangashley]

You check that FANTASY_1 is still in the watchlist and that HORROR_1 was added to the watched list. You might also consider checking that HORROR_1 was not added to watchlist with assert HORROR_1 not in updated_data["watchlist"].

You could also check that FANTASY_2 which was already in the watched list wasn't removed.

[yangashley]

👍 great job using a function you already wrote to keep your code DRY.

[yangashley]

Like my comment above, the three continues give me pause.

You invoked the method to get the most watched genre on line 114. You can also invoke the method get_friends_unique_watched to make the code more concise.

When you have those two values, you can check if each movie in the friends_unique_movies list has a genre equal to the value of the most watched genre. If they're equal, then you can append that movie to recommended_movies.

Something like

recommended_movies = []
favorite_genre = get_most_watched_genre(user_data)
friends_unique_movies = get_friends_unique_watched(user_data)

for movie in friends_unique_movies:
        if favorite_genre == movie["genre"]:
            recommended_movies.append(movie)
return recommended_movies

[yangashley]

Since remove has linear time complexity, how can we go about writing the logic so that we append a movie to the list instead?

If you get a list of the movies watched by friends (with a helper method like suggested above) and then check that a user's favorite movies are not in that list, then we could append that movie to a new list like recommendations.

Then you wouldn't need a whole copy of user_data["favorites"] and would not incur the linear time complexity of removing an element from a list (you'll have the constant time complexity of append).

This might look like:

def get_rec_from_favorites():
    recommendations = []
    friends_watched = get_friends_watched_movies()

    for favorite in user_data["favorites"]:
        if not favorite in friends_watched:
           recommendations.append(favorite)
    return recommendations

[yangashley]

recommended_list is created but never used. Delete objects that are not accessed.

[yangashley]

We should take care to not add in unnecessary changes to a commit so that our project stays neat. Looks like an unneeded blank line was added on line 1 here.