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.

Sign up for GitHub

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 Tiffany Rogers #168

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

Panthers Tiffany Rogers #168

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
849c893
wave 1 and 2
Tiffany-Rogers-Dev Sep 27, 2022
e9e8e9e
1,2,3
Tiffany-Rogers-Dev Sep 29, 2022
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
3 changes: 3 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
# Viewing Party


## Command to start Virtual Enviornment
source ven/bin/activate
## Skills Assessed

Solving problems with...
Expand Down
12 changes: 6 additions & 6 deletions play_tester.py
View file
Open in desktop
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-----")
Copy link

Choose a reason for hiding this comment

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

Committing changes to this file & the README is no issue for this project, but something to think about going forward is that we generally don't want to commit temporary changes for testing, or changes to files that we don't intend to purposefully share with other folks. This can mean having to be more selective when staging files for commit over being able to use git add .

# 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
28 changes: 17 additions & 11 deletions tests/test_wave_01.py
View file
Open in desktop
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,15 @@ 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.")
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
Comment on lines +122 to +124
Copy link

Choose a reason for hiding this comment

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

Nice checks for all the movie's data!


# *******************************************************************************************
# ****** 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 +146,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.")
assert updated_data["watched"][1]["title"] == MOVIE_TITLE_1
assert updated_data["watched"][1]["genre"] == GENRE_1
assert updated_data["watched"][1]["rating"] == RATING_1
Comment on lines +149 to +151
Copy link

Choose a reason for hiding this comment

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

Our problem statement doesn't specify what order a movie should be added to the watched list in. If movies were added to the beginning to the list rather than the end, these tests would fail. A more flexible way we could check if a movie is in watched, is to check if the entire movie dictionary exists in the list:

assert HORROR_1 in updated_data["watched"]

When writing tests, we want to check all the relevant data to ensure there weren't unexpected side affects. Since there are other movies in the watchlist and watched lists, we could also add assertions for those:

assert FANTASY_2 in updated_data["watched"]
assert FANTASY_1 in updated_data["watchlist"]


# *******************************************************************************************
# ****** 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
8 changes: 4 additions & 4 deletions tests/test_wave_02.py
View file
Open in desktop
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 @@ -14,7 +14,7 @@ def test_calculates_watched_average_rating():
assert average == pytest.approx(3.58333)
assert janes_data == clean_wave_2_data()

@pytest.mark.skip()
# @pytest.mark.skip()
def test_empty_watched_average_rating_is_zero():
# Arrange
janes_data = {
Expand All @@ -27,7 +27,7 @@ def test_empty_watched_average_rating_is_zero():
# Assert
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
14 changes: 8 additions & 6 deletions tests/test_wave_03.py
View file
Open in desktop
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,14 @@ def test_friends_unique_movies_not_duplicated():
# Assert
assert len(friends_unique_movies) == 3

raise Exception("Test needs to be completed.")
assert INTRIGUE_3 in friends_unique_movies
assert HORROR_1 in friends_unique_movies
assert FANTASY_4 in friends_unique_movies
# *************************************************************************************************
# ****** 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
98 changes: 94 additions & 4 deletions viewing_party/party.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,107 @@
# ------------- WAVE 1 --------------------
import statistics
from statistics import mode
Comment on lines +2 to +3
Copy link

Choose a reason for hiding this comment

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

Since we don't use statistics on its own, we could remove the line import statistics without affecting how the code works.



def create_movie(title, genre, rating):
pass
movies = {
Copy link

Choose a reason for hiding this comment

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

When we pluralize a variable name, that usually means the variable is pointing to a collection holding many objects. If movies isn't a collection of multiple movies, we probably want to give it a singular name to better reflect what it holds.

'title':title,
'genre':genre,
'rating':rating,
}
if not title or not genre or not rating:
return None
Comment on lines +12 to +13
Copy link

Choose a reason for hiding this comment

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

Nice one-liner to validate all the inputs!

In this implementations, the movies variable will be created and allocated space before we know if the inputs are valid. I would recommend flipping the order and placing the validity check first so that movies is only created if it will be needed.

else:
return movies


def add_to_watched(user_data, movie):
user_data["watched"].append(movie)

return user_data
Comment on lines +18 to +21
Copy link

Choose a reason for hiding this comment

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

Finding a balance of whitespace inside of a function can be hard, and is often subjective. That said, for a very short function like this with a single statement and return, I would recommend removing the new line on line 20 since this function is so small that everything is tightly related. This applies to the add_to_watchlist function below as well.



def add_to_watchlist(user_data,movie):
user_data["watchlist"].append(movie)

return user_data


def watch_movie(user_data, title):
movie_to_watch = user_data["watchlist"]
for movie in movie_to_watch:
if title == movie["title"]:
user_data["watched"].append(movie)
user_data["watchlist"].remove(movie)
return user_data
Comment on lines +32 to +36
Copy link

Choose a reason for hiding this comment

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

This function works, but it contains an "anti-pattern" that's important to be aware of. An element of the list that is being iterated over is being removed inside the for loop. This won't cause problems in this particular case, but consider this slightly different implementation:

def watch_movie(user_data, title):
    for i in range(len(user_data["watchlist"])):
        item = user_data["watchlist"][i]

        if item["title"] == title:
            user_data["watched"].append(item)
            user_data["watchlist"].remove(item)
    return user_data

If we were to test this in a scenario where user_data["watchlist"] has 2 elements, and the first is the movie that we are moving to user_data["watched"], the second iteration through the loop will throw an index error, because it will try to access the 2nd element in user_data["watchlist"], but there no longer is a second element.

As a coding pattern, this is one that I recommend avoiding because it has so much potential for unforeseen errors. One fix is to return immediately after making the swap (ie, move line 36 inside the if-block so that the loop ends as soon as the swap is made). Another fix is to use the loop to find the item, but not make the swap until after the loop ends.


# -----------------------------------------
# ------------- WAVE 2 --------------------
# -----------------------------------------
def get_watched_avg_rating(user_data):
sum = 0
Copy link

Choose a reason for hiding this comment

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

We want to be careful of choosing variable names that are already used by a language for built in functions. In python, everything is a variable, including function names. Python has a built in function named sum. When we use the same name and assign it to a new value like we're doing here, we lose access to the built in function. We would have to take steps to store a reference to the built in function in a new variable before reassigning its original name to still have access to it.

for i in range (len(user_data['watched'])):
movie = user_data['watched'][i]
movie_r = movie['rating']
sum += movie_r
Comment on lines +43 to +46
Copy link

Choose a reason for hiding this comment

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

This implementation only reads i to get a movie out of user_data['watched']. If we used a different kind of loop, we could simplify our syntax a little and remove i:

    for movie in user_data['watched']:   
        sum += movie['rating']


if (len(user_data['watched'])) == 0:
Copy link

Choose a reason for hiding this comment

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

Similar to feedback in create_movie, I'd recommend moving this validation check to the top of the function so that we avoid processing and memory allocation we won't need if the watched list is empty

return 0.0
else:
return sum / len(user_data['watched'])
Copy link

Choose a reason for hiding this comment

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

Nice approach! One of many possible implementations could be to use a list comprehension and the sum function:

# The list comprehension inside the `sum` call pulls the value of the 
# `rating` key out of each item in the list `user_data['watched']`
total = sum(movie['rating'] for movie in user_data['watched'])
average_rating = total / len(user_data['watched'])
return average_rating


# -----------------------------------------
def get_most_watched_genre(user_data):
movie_g_list = []
Copy link

Choose a reason for hiding this comment

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

I would recommend staying away from single letter abbreviations in variable names. Especially in larger projects and even long functions, it could be hard to know what a letter stands for without needing to closely read the surrounding code.
Since this variable will hold a list of genre names, it could be clearer for readers if we used names like movie_genres, genres_list, or genre_names, as some examples.

for i in range (len(user_data['watched'])):
movie = user_data['watched'][i]
movie_g = movie['genre']
movie_g_list.append(movie_g)
Comment on lines +54 to +58
Copy link

Choose a reason for hiding this comment

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

List comprehensions are also really handy for filling a list from existing data. Another way we could fill movie_g_list could be:

movie_g_list = [movie['genre'] for movie in user_data['watched']]

if user_data['watched'] == []:
return None
else:
return(mode(movie_g_list))
Copy link

Choose a reason for hiding this comment

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

Great use of mode!


# -----------------------------------------
# ------------- WAVE 3 --------------------
# -----------------------------------------
def get_unique_watched(user_data):
friends = user_data['friends']
friends_mediocre_movies = []
my_watched = user_data['watched']
cool_movies_watched = []
Comment on lines +68 to +71
Copy link

Choose a reason for hiding this comment

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

These variables are holding references to data we have from the input. Especially if memory is limited, we often prefer to read data from the original structure and create a copy only if we need one for editing or because it would make the code easier to read.

There's a tradeoff of creating named variables, descriptive names can make code much easier to understand, but at the same time, the more variables we have, the more values we need to keep in mind when reading through the code.

Finding the right balance depends on what works for you and your team, and the restrictions of the system you're programming for. Another possible implementation that factors out some of these variables could look like:

    friends_mediocre_movies = []
    for friend in user_data['friends']:
        for friends_movie in friend['watched']:
            friends_mediocre_movies.append(friends_movie)

    unique_movies = []
    for movie in user_data['watched']:
        if not movie in friends_mediocre_movies and movie not in unique_movies:
            unique_movies.append(movie)
            
    return unique_movies

unique_movies = []
for my_movies in my_watched:
cool_movies_watched.append(my_movies)
Comment on lines +73 to +74
Copy link

Choose a reason for hiding this comment

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

Python's list comprehensions give us another option to fill a list from existing data:

# We can read the list comprehension on the right side of the assignment operator as: 
# "For each movie in user_data['watched'] add movie to a new list"
cool_movies_watched = [movie for movie in user_data['watched']]


for friends_watched in friends:
for friends_movies in friends_watched['watched']:
friends_mediocre_movies.append(friends_movies)
Comment on lines +76 to +78
Copy link

Choose a reason for hiding this comment

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

This doesn't change the function's overall performance, but another option to add values from one list to another is the extend function:

   for friends_watched in friends:
       friends_mediocre_movies.extend(friends_watched['watched')


for movie in cool_movies_watched:
if not movie in friends_mediocre_movies and movie not in unique_movies:
Copy link

Choose a reason for hiding this comment

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

We have 2 expressions in this statement not movie in friends_mediocre_movies and movie not in unique_movies. How does the location of the not operator change how these are evaluated?

unique_movies.append(movie)
return unique_movies
Copy link

Choose a reason for hiding this comment

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

Great approach with nested loops! Another of many possible implementations could use the Set data structure to solve this – if we created a Set that held our user_data["watched"] movies and another Set that held all the movies in our friends' watched lists, then we could take the difference of those sets!

my_movies = set(user_data["watched"])

friend_movies = set()
for friend in user_data["friends"]:
        for movie in friend["watched"]:
                friend_movies.add(movie)

unique_watched_set = my_movies - friend_movies
return list(unique_watched_set)



def get_friends_unique_watched(user_data):
Copy link

Choose a reason for hiding this comment

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

The feedback for get_unique_watched applies to this function as well.

friends = user_data['friends']
friends_mediocre_movies = []
my_watched = user_data['watched']
cool_movies_watched = []
unique_movies = []
for my_movies in my_watched:
cool_movies_watched.append(my_movies)

for friends_watched in friends:
for friends_movies in friends_watched['watched']:
friends_mediocre_movies.append(friends_movies)
Comment on lines +95 to +97
Copy link

Choose a reason for hiding this comment

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

We have some repeated code in get_unique_watched and here to fill a list with all our friend's movies. How could a helper function reduce this repeated code?


for movie in friends_mediocre_movies:
if not movie in cool_movies_watched and movie not in unique_movies:
unique_movies.append(movie)
return unique_movies




# -----------------------------------------
Expand All @@ -19,5 +110,4 @@ def create_movie(title, genre, rating):

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

# -----------------------------------------