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

Ocelots - Diana A. #20

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

Ocelots - Diana A. #20

wants to merge 16 commits into from

Conversation

diarreola
Copy link

No description provided.

@@ -11,6 +11,10 @@
GENRE_1 = "Horror"
RATING_1 = 3.5

MOVIE_TITLE_2 = "Ratatouille"

Choose a reason for hiding this comment

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

Way to go, even adding additional testing data!


# Assert
assert len(updated_data["watched"]) == 2
assert updated_data["watched"][1]["title"] == MOVIE_TITLE_1

Choose a reason for hiding this comment

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

Excellent thoroughness, testing all of the attributes here for completeness

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

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

Choose a reason for hiding this comment

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

Interesting use of casting to bool to cover "" and None



def watch_movie(user_data, title):
for movie in user_data["watchlist"]:

Choose a reason for hiding this comment

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

This is working here, but an important note is that it is generally dangerous practice to modify a data structure that you are using to loop over. This can lead to bugs in more complex code. One alternative could be making a copy of what you get from " movie in user_data["watchlist"]:" so that that collection does not change while you modify user_data["watchlist]" from inside the loop.


watched_genres = {}
for movie in user_data["watched"]:
watched_genres[movie["genre"]] = watched_genres.get(movie["genre"], 0) + 1

Choose a reason for hiding this comment

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

Why was 1 added here?

def get_unique_watched(user_data):
watched_movies_list = user_data["watched"]
friends_watched_movies_list = get_friends_watched_movies(user_data)
return [movie for movie in watched_movies_list if movie not in friends_watched_movies_list]

Choose a reason for hiding this comment

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

This is a really clever single line! I would say that in general it's more useful for yourself or other developers to write out the steps, so that it's easier to understand-- and to debug and maintain in the future.

Choose a reason for hiding this comment

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

Also, now that we've had some more experience, can you think about how sets could have been used here, and in similar code in your project?


def remove_movie_duplicates(friends_watched_movies_list):
friends_watched_movie_titles = {}
for movie in friends_watched_movies_list:

Choose a reason for hiding this comment

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

Similar comments about the dangers of modifying a collection while you are iterating over it

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