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

Snow Leopards- Ja Hopkins #153

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions play_tester.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,16 @@
pp = pprint.PrettyPrinter(indent=4)

# play testing section
print("\n-----Wave 01 test data-----")
pp.pprint(HORROR_1)
pp.pprint(FANTASY_1)
pp.pprint(FANTASY_2)
# print("\n-----Wave 01 test data-----")
# pp.pprint(HORROR_1)
# pp.pprint(FANTASY_1)
# pp.pprint(FANTASY_2)

# print("\n-----Wave 02 user_data-----")
# pp.pprint(clean_wave_2_data())

#print("\n-----Wave 03 user_data-----")
#pp.pprint(clean_wave_3_data())
# print("\n-----Wave 03 user_data-----")
# pp.pprint(clean_wave_3_data())

# Wave 04 user data
#print("\n-----Wave 04 user_data-----")
Expand Down
29 changes: 18 additions & 11 deletions tests/test_wave_01.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
from viewing_party.party import *
from tests.test_constants import *

@pytest.mark.skip()
# @pytest.mark.skip()
def test_create_successful_movie():
# Arrange
movie_title = MOVIE_TITLE_1
Expand All @@ -19,7 +19,7 @@ def test_create_successful_movie():
assert new_movie["genre"] == GENRE_1
assert new_movie["rating"] == pytest.approx(RATING_1)

@pytest.mark.skip()
# @pytest.mark.skip()
def test_create_no_title_movie():
# Arrange
movie_title = None
Expand All @@ -32,7 +32,7 @@ def test_create_no_title_movie():
# Assert
assert new_movie is None

@pytest.mark.skip()
# @pytest.mark.skip()
def test_create_no_genre_movie():
# Arrange
movie_title = "Title A"
Expand All @@ -45,7 +45,7 @@ def test_create_no_genre_movie():
# Assert
assert new_movie is None

@pytest.mark.skip()
# @pytest.mark.skip()
def test_create_no_rating_movie():
# Arrange
movie_title = "Title A"
Expand All @@ -58,7 +58,7 @@ def test_create_no_rating_movie():
# Assert
assert new_movie is None

@pytest.mark.skip()
# @pytest.mark.skip()
def test_adds_movie_to_user_watched():
# Arrange
movie = {
Expand All @@ -79,7 +79,7 @@ def test_adds_movie_to_user_watched():
assert updated_data["watched"][0]["genre"] == GENRE_1
assert updated_data["watched"][0]["rating"] == RATING_1

@pytest.mark.skip()
# @pytest.mark.skip()
def test_adds_movie_to_user_watchlist():
# Arrange
movie = {
Expand All @@ -100,7 +100,7 @@ def test_adds_movie_to_user_watchlist():
assert updated_data["watchlist"][0]["genre"] == GENRE_1
assert updated_data["watchlist"][0]["rating"] == RATING_1

@pytest.mark.skip()
# @pytest.mark.skip()
def test_moves_movie_from_watchlist_to_empty_watched():
# Arrange
janes_data = {
Expand All @@ -119,12 +119,16 @@ def test_moves_movie_from_watchlist_to_empty_watched():
assert len(updated_data["watchlist"]) == 0
assert len(updated_data["watched"]) == 1

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

👍

assert updated_data["watched"][0]["genre"] == GENRE_1
assert updated_data["watched"][0]["rating"] == RATING_1
# *******************************************************************************************
# ****** Add assertions here to test that the correct movie was added to "watched" **********
# *******************************************************************************************

@pytest.mark.skip()
# @pytest.mark.skip()
def test_moves_movie_from_watchlist_to_watched():
# Arrange
movie_to_watch = HORROR_1
Expand All @@ -143,12 +147,15 @@ def test_moves_movie_from_watchlist_to_watched():
assert len(updated_data["watchlist"]) == 1
assert len(updated_data["watched"]) == 2

raise Exception("Test needs to be completed.")
# raise Exception("Test needs to be completed.")

assert updated_data["watched"][1] == movie_to_watch
assert movie_to_watch in updated_data["watched"]
Comment on lines +152 to +153

Choose a reason for hiding this comment

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

These two checks are more or less doing the same thing. Consider using three in checks, one for checking which movie is remaining int the watchlist list, and two for each of the movies we expect to find in the watched list

# *******************************************************************************************
# ****** Add assertions here to test that the correct movie was added to "watched" **********
# *******************************************************************************************

@pytest.mark.skip()
# @pytest.mark.skip()
def test_does_nothing_if_movie_not_in_watchlist():
# Arrange
movie_to_watch = HORROR_1
Expand Down
6 changes: 3 additions & 3 deletions tests/test_wave_02.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
from viewing_party.party import *
from tests.test_constants import *

@pytest.mark.skip()
# @pytest.mark.skip()
def test_calculates_watched_average_rating():
# Arrange
janes_data = clean_wave_2_data()
Expand All @@ -27,7 +27,7 @@ def test_empty_watched_average_rating_is_zero():
# Assert

Choose a reason for hiding this comment

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

👀 It looks like you missed uncommenting the skip on the test on line 18. When I did so, the test passed. 👍

assert average == pytest.approx(0.0)

@pytest.mark.skip()
# @pytest.mark.skip()
def test_most_watched_genre():
# Arrange
janes_data = clean_wave_2_data()
Expand All @@ -39,7 +39,7 @@ def test_most_watched_genre():
assert popular_genre == "Fantasy"
assert janes_data == clean_wave_2_data()

@pytest.mark.skip()
# @pytest.mark.skip()
def test_genre_is_None_if_empty_watched():
# Arrange
janes_data = {
Expand Down
16 changes: 10 additions & 6 deletions tests/test_wave_03.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
from viewing_party.party import *
from tests.test_constants import *

@pytest.mark.skip()
# @pytest.mark.skip()
def test_my_unique_movies():
# Arrange
amandas_data = clean_wave_3_data()
Expand All @@ -16,7 +16,7 @@ def test_my_unique_movies():
assert INTRIGUE_2 in amandas_unique_movies
assert amandas_data == clean_wave_3_data()

@pytest.mark.skip()
# @pytest.mark.skip()
def test_my_not_unique_movies():
# Arrange
amandas_data = clean_wave_3_data()
Expand All @@ -28,7 +28,7 @@ def test_my_not_unique_movies():
# Assert
assert len(amandas_unique_movies) == 0

@pytest.mark.skip()
# @pytest.mark.skip()
def test_friends_unique_movies():
# Arrange
amandas_data = clean_wave_3_data()
Expand All @@ -43,7 +43,7 @@ def test_friends_unique_movies():
assert FANTASY_4 in friends_unique_movies
assert amandas_data == clean_wave_3_data()

@pytest.mark.skip()
# @pytest.mark.skip()
def test_friends_unique_movies_not_duplicated():
# Arrange
amandas_data = clean_wave_3_data()
Expand All @@ -55,12 +55,16 @@ def test_friends_unique_movies_not_duplicated():
# Assert
assert len(friends_unique_movies) == 3

raise Exception("Test needs to be completed.")
# raise Exception("Test needs to be completed.")

assert friends_unique_movies[-2] == HORROR_1
assert friends_unique_movies[0] == FANTASY_4
Comment on lines +60 to +61

Choose a reason for hiding this comment

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

There weren't any particular specifications as to the ordering of the movies in the list of results. As a result, I would tend to use an in check for the movies we expect to see, since that won't require knowing at what exact position each movie ended up. This lets us enforce the required behaviors in the test without being overly strict.

In fact, the checks from the previous test are exctly what we would need to add.

    assert INTRIGUE_3 in friends_unique_movies
    assert HORROR_1 in friends_unique_movies
    assert FANTASY_4 in friends_unique_movies

Since this test already has a check for the list having 3 items, if we then check for 3 expected items and find them all, this also means that each item appears in the result only once (no duplication).

# this assert makes sures that the second to last item is not INTRIGUE_3 which would mean it was duplicated
# *************************************************************************************************
# ****** Add assertions here to test that the correct movies are in friends_unique_movies **********
# **************************************************************************************************

@pytest.mark.skip()
# @pytest.mark.skip()
def test_friends_not_unique_movies():
# Arrange
amandas_data = {
Expand Down
6 changes: 3 additions & 3 deletions tests/test_wave_04.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
from viewing_party.party import *
from tests.test_constants import *

@pytest.mark.skip()
# @pytest.mark.skip()
def test_get_available_friend_rec():
# Arrange
amandas_data = clean_wave_4_data()
Expand All @@ -16,7 +16,7 @@ def test_get_available_friend_rec():
assert FANTASY_4b in recommendations
assert amandas_data == clean_wave_4_data()

@pytest.mark.skip()
# @pytest.mark.skip()
def test_no_available_friend_recs():
# Arrange
amandas_data = {
Expand All @@ -38,7 +38,7 @@ def test_no_available_friend_recs():
# Assert
assert len(recommendations) == 0

@pytest.mark.skip()
# @pytest.mark.skip()
def test_no_available_friend_recs_watched_all():
# Arrange
amandas_data = {
Expand Down
122 changes: 120 additions & 2 deletions viewing_party/party.py
Original file line number Diff line number Diff line change
@@ -1,23 +1,141 @@
# ------------- WAVE 1 --------------------


def create_movie(title, genre, rating):
pass
# if truthy return a dictionary
# dictionary contains 3 key-value pairs
# return none if falsy
title_genre_rating = {}
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.

Nice truthy checks. But consider reversing the sense of the comparison so that the more error-like condition is handled within the conditional check, and the main logic (making the dictionary) doesn't need to be nested.

Structuring the checks to look for the more error-like conditions is a pattern referred to as a guard clause, and it's a common way of organizing our code.

    if not title or not genre or not rating:
        # code to run when one of the inputs is falsy

    # code to run when all inputs are truthy

title_genre_rating.update({'title': title, 'genre': genre, 'rating': rating})

Choose a reason for hiding this comment

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

Notice that the argument to update is a newly created dictionary. We could assign that new dictionary directly to our variable, or even return it, rather than updating the empty dictionary we started with.

        title_genre_rating = {'title': title, 'genre': genre, 'rating': rating}

return(title_genre_rating)

Choose a reason for hiding this comment

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

return isn't a function, so we don't need the parentheses around the value we're returning. In python, the only reason we'd put parentheses is if we were returning the result of a complex expression and wanted to be able to implicitly wrap the line. Notice we would also put a space after the return to keep it from looking as much like a function call.

    return ("this" + "is" + "a" + "long" + "expression"
        + "that" + "I" + "want" + "to" + "wrap")

return None

Choose a reason for hiding this comment

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

Taking all that feedback, we might end up with a function like this:

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

    return {'title': title, 'genre': genre, 'rating': rating}


def add_to_watched(user_data, movie):

Choose a reason for hiding this comment

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

👍

user_data["watched"].append(movie)
return user_data

# user_data = {watched: [{movie}{moveie}]}
# add each movie to the watched key

def add_to_watchlist(user_data, movie):

Choose a reason for hiding this comment

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

👍

# user_data = {watchlist: [{movies}{user}]{wants}{to_watch}}
# add movie into the watchlist inside of user data
# return user_data
user_data["watchlist"].append(movie)
return(user_data)

Choose a reason for hiding this comment

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

As above, no need for the parens on the return here (or throughout the rest of the functions).


def watch_movie(user_data, title):
# user_data = {watchlist: watched:}
# if title is in watch_list then add to watched:
# return user_data
# if not in watch list:
# return user_data

for movie in user_data["watchlist"]:
if title in movie.values():

Choose a reason for hiding this comment

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

This could potentially find the title under some other key than the expected "title" key. We know what key we're interested in checking, so we should check it directly.

        if title == movie["title"]:

user_data["watchlist"].remove(movie)
user_data["watched"].append(movie)

Choose a reason for hiding this comment

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

Consider breaking out of the looping after performing the remove. Remember that modifying a loop wihle iterating over it (the watchlist list here) can result in unexpected behavior. It's probably not a problem here, since we only expect to find one of the movie in the list, but we can make that explicit by using break to terminate the iteration as soon as we modify the list.

Also, a blank line after this would help visually separate the return (which is outside the loop) from the loop body.

return(user_data)



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

def get_watched_avg_rating(user_data):
# user data = {watched: [{movie2}, {movie2}]}
user_rating = []

if len(user_data["watched"]) == 0:

Choose a reason for hiding this comment

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

Nice handling for the case of an empty list.

A slightly more pythonic way to check for whether a list is empty is

    if not some_list_var:
        # code to handle empty list

So here,

    if not user_data["watched"]:

average_rating = 0.0
return average_rating
else:

Choose a reason for hiding this comment

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

Since the if condition results in a return (effectively a guard clause), we can leave the else off, and unindent the following code. That helps emphasize that it's the more important code.

for movie in user_data["watched"]:
user_rating.append(movie["rating"])
Comment on lines +55 to +56

Choose a reason for hiding this comment

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

We haven't talked about list comprehensions in python, but there's a special syntax that lets us effectively append a set of values very concisely.

The following would initialize user_rating to be a list containing the ratings of each movie from the watched list.

    user_rating = [movie["rating"] for movie in user_data["watched"]]

The general pattern lets us take some code like

new_list = []
for item in old_list:
    if should_process(item):
        new_list.append(process(item))

and write it like:

new_list = [process(item) for item in old_list if should_process(item)]

average_rating = (sum(user_rating)) / (len(user_rating))

Choose a reason for hiding this comment

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

Notice that since this is indented under the loop, we're recalculating the average each time through the loop. We only need to do this once (after the loop), after we've added all of the ratings to the list in the loop.

# print(user_rating)
# print(average_rating)
return(average_rating)

def get_most_watched_genre(user_data):

genres_watched = {}

if not user_data["watched"]:

Choose a reason for hiding this comment

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

👍 Nice and Pythonic!

return None

for movie in user_data["watched"]:
if movie["genre"] not in genres_watched:
genres_watched[movie["genre"]] = 1
elif movie["genre"] in genres_watched:

Choose a reason for hiding this comment

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

A key is either in a dict or it's not. So we can use a else: without an expression, rather than an elif:.

genres_watched[movie["genre"]] += 1

num_of_genres_watched = genres_watched.values()
max_num_genres_watched = max(num_of_genres_watched)

Choose a reason for hiding this comment

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

Nice use of max to find the highest count, but notice then that we must iterate back through the data again and to find a genre that had this count. Alternatively, we could iterate over the frequency map ourselves to find the max count, keeping track of the key (genre) ourself.

We'll see that there's really not a performance difference, though, and arguably this code is a little simpler. (but writing a max finder ourself is great practice!).


for genre, times_watched in genres_watched.items():
if times_watched == max_num_genres_watched:
return genre



# -----------------------------------------
# ------------- WAVE 3 --------------------
# -----------------------------------------

def get_unique_watched(user_data):
# user_data = {watched :[{"title: title"}, {"title": title}], friends: [{watched_key:[{"title:title"}{title:title}]}{watched_key:[{title:title}{title:title}]}]}
# each movie dict in watched_key has a title

unique_movies = []
movies_watched_by_friends = []

for friend in user_data["friends"]:
for movie_details in friend["watched"]:
movie_title = movie_details["title"]
movies_watched_by_friends.append(movie_title)
Comment on lines +95 to +98

Choose a reason for hiding this comment

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

Nice way to build up a list of movie titles the friends have watched. Notice that since you decided to use the title as representative of the entire movie record ( areasonable decision given our data), we could use a set for the movies_watched_by_friends collection rather than a list. This would improve the complexity of the in check on line 102 (since sets have constant time lookup, while a list is linear time).

# print(movie_details["title"])
for movie in user_data["watched"]:
movie_watched = movie["title"]
if movie_watched not in movies_watched_by_friends:
unique_movies.append(movie)
# watched_titles.append(movie["title"])
return(unique_movies)

Choose a reason for hiding this comment

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

No need for () around the return value. We can wrap the value with () if we are returning the result of a long expression and need to line wrap, but otherwise, we generally leave them off. We would also leave a space between return and the () around the expression, since return isn't a function and we don't want it to look like we're doing a function call.


def get_friends_unique_watched(user_data):
all_movies_watched = []
unique_movies_watched_by_friends = []

for movie in user_data["watched"]:
movie_watched = movie["title"]
# add all the movies watched to new list
all_movies_watched.append(movie_watched)
Comment on lines +111 to +114

Choose a reason for hiding this comment

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

As above, since you're using the title as a proxy for the entire moview, we could use a set for all_movies_watched for improved in check performance.

for friend in user_data["friends"]:
for movie_details in friend["watched"]:
movie_title = movie_details["title"]
if (movie_title not in all_movies_watched and
movie_details not in unique_movies_watched_by_friends):

Choose a reason for hiding this comment

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

Nice way to filter out a movie that's already been added from another friend. Here, we do need to build a list of movie detail records, not just the title. So unique_movies_watched_by_friends must be a list. However, if we still wanted to gain the constant in lookup even for the result list, we could create another set in which we store only the titles of the movies we've added to unique_movies_watched_by_friends, and look up a movie's title in that set before adding it to the result list, and adding the title to the set if we do end up adding a movie to the result list.

As it is, doing the in checks (which are linear) in a loop (essentially linear) gives us an overall quadratic time complexity. But we could get this down to linear by building that extra set, since the operations in the loop would become constant.

# check if details are unqie friends list to avoid duplicates
# create list of dictionary containing movie details
unique_movies_watched_by_friends.append(movie_details)
return unique_movies_watched_by_friends



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

def get_available_recs(user_data):
recommended_movies = []
friends_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.

👍 Great reuse of your get_friends_unique_watched function to simplify the implementation here.

for movie in friends_watched:
if movie["host"] in user_data["subscriptions"]:
recommended_movies.append(movie)
return recommended_movies

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