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

Kunzite - Gabby Y. and Allie S. #45

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

Kunzite - Gabby Y. and Allie S. #45

wants to merge 21 commits into from

Conversation

gnyoung
Copy link

@gnyoung gnyoung commented Mar 30, 2023

No description provided.

Copy link

@spitsfire spitsfire left a comment

Choose a reason for hiding this comment

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

Great job, Gabby and Allie! Very clean, easy to read code! I've left a few suggestions below, but overall your logic looks good!

@@ -1,23 +1,137 @@
# ------------- WAVE 1 --------------------

def create_movie(title, genre, rating):
pass
if (not title) or (not genre) or (not rating):

Choose a reason for hiding this comment

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

We only need parentheses around conditional statements when we need multiple conditions to be finished first, then move on to another group. For example, if (not title or not genre) and not rating; otherwise, forgo parentheses. It's not very Pythonic.

Comment on lines +11 to +14
new_movie = {}
new_movie["title"] = title
new_movie["genre"] = genre
new_movie["rating"] = rating

Choose a reason for hiding this comment

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

This works! We can also do this all at once, too inside new_movie

return new_movie


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.

👍 perf

return user_data


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.

👍


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


def get_watched_avg_rating(user_data):

Choose a reason for hiding this comment

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

👍

Comment on lines +35 to +39
for movie in user_data["watchlist"]:
if title == movie["title"]:
move_movie = movie
user_data["watchlist"].remove(movie)
user_data["watched"].append(move_movie)

Choose a reason for hiding this comment

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

Careful here! We are removing items from a list while iterating through it. This can result in items being skipped over because of a shift in index positions.

How best to combat this can depend. We could make a copy of the watchlist and remove from one while we loop through the other. Or perhaps we can break once we finish line 39. But then that begs the question, what if the same movie is inside the list?

Something to think about!

Comment on lines +178 to +183
assert updated_data["watchlist"] == []
assert updated_data["watched"] == [{
"title": MOVIE_TITLE_1,
"genre": GENRE_1,
"rating": RATING_1
}]

Choose a reason for hiding this comment

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

👍

Comment on lines +209 to +212
assert updated_data["watchlist"] == [
FANTASY_1
]
assert updated_data["watched"] == [FANTASY_2, movie_to_watch]

Choose a reason for hiding this comment

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

👍

Comment on lines +67 to +73
assert friends_unique_movies == [
{'title': 'The Programmer: An Unexpected Stack Trace',
'genre': 'Fantasy', 'rating': 4.0},
{'title': 'It Came from the Stack Trace',
'genre': 'Horror', 'rating': 3.5},
{'title': 'Zero Dark Python', 'genre': 'Intrigue', 'rating': 3.0}
]

Choose a reason for hiding this comment

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

👍

Comment on lines +64 to +68
recommendations = get_new_rec_by_genre(sonyas_data)

# Assert
assert recommendations == []
assert len(recommendations) == 0

Choose a reason for hiding this comment

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

👍

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.

3 participants