Skip to content

Anees Quateja & Liubov Davidova - phoenix class #19

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

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

Conversation

aneesquateja
Copy link

No description provided.

Copy link

@apradoada apradoada left a comment

Choose a reason for hiding this comment

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

🟢 GREEN! 🟢

Hey y'all!

Overall, this looks great! It passes all the tests and I didn't find any major mishaps, so you are receiving a green for the viewing party project!

Two things I would love for y'all to focus on/think about as you move forward with your programming journey!

  1. The first is consistency! Sometimes y'all would pull out a specific list from the dictionary and other times y'all wouldn't! Either way works (with a slight preference for accessing the lists directly) as long as y'all are treating the lists the same way each time!

  2. When you start to see yourself with a triply nested loop, see if you are able to pare it down. While triply nested loops can and do work, they can drastically slow down our program. In most cases, if we have that level of nesting, we are able to shrink it down to just one or two loops. If we aren't able to, it might be a sign to rethink our approach!

Great job, y'all!
-Adrian

@pytest.mark.skip()
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

Choose a reason for hiding this comment

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

These tests look great!

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

@pytest.mark.skip()
assert updated_data["watched"][1] == 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 +63 to +65
assert friends_unique_movies[0]["title"] == FANTASY_4["title"]
assert friends_unique_movies[1]["title"] == HORROR_1["title"]
assert friends_unique_movies[2]["title"] == INTRIGUE_3["title"]

Choose a reason for hiding this comment

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

These tests are a great start! Just a reminder that right now, y'all are only checking to see if the movie titles are the same. A more comprehensive test would actually just check to see if the movies themselves are the same. For example:

 assert friends_unique_movies[0] == FANTASY_4

recommendations = get_new_rec_by_genre(sonyas_data)

# Assert
assert len(recommendations) == 0

Choose a reason for hiding this comment

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

👍

pass
movie = {}

if title == None or title == "" or genre == None or genre == "" or rating == None or rating == "":

Choose a reason for hiding this comment

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

This approach is great at making sure we have all our bases covered! One thing to remember, however, is that both None and an empty string are falsy conditions for strings. Therefore, the conditional

 if title == None or title == "":

is actually the same conditional as

 if not title:

This allows us to make both checks at once as opposed to having to separate them out!

def get_available_recs(user_data):
recommended_movies = []

friends_unique_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 use of the function you wrote before!

if subscription == movie["host"]:
recommended_movies.append(movie)

return recommended_movies

Choose a reason for hiding this comment

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

This function looks good!

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

def get_new_rec_by_genre(user_data):
most_watched_genre_movies = []

Choose a reason for hiding this comment

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

This is just a small thing, but a slightly more descriptive variable here might be better. Maybe instead of using "movies", you can use "recs" to signify that what you are outputting are recommendations rather than just random movies!

if movie["genre"] == user_most_watched_genre:
most_watched_genre_movies.append(movie)

return most_watched_genre_movies

Choose a reason for hiding this comment

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

👍


for movie in unique_watched_user_movies:
for favorite in user_data["favorites"]:
if favorite["title"] == movie["title"]:

Choose a reason for hiding this comment

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

Remember that instead of checking each title in unique_watched_user_movies, we can compare specific movies to each other.

I love the way you created unique_watched_user_movies using the helper function from wave 4. But now that we have that particular list, what if we just looped through the user_data["favorites"] and checked to see if each movie was in unique_watched_user_movies. This won't necessarily save us on complexity(remember that in will technically count as a loop in most cases), but it will save us a nested loop!

In the grand scheme of things, when we have a nested structure like the one in this project, it's a good idea to check entire dictionaries against each rather than check dictionary key-value pairs against each other!

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