Sapphire-Selene C.

[secasti]

No description provided.

[tildeee]

(Tagging @charliemushishi to ensure visibility)

Great work on this project, y'all! Your project code gets a "green" in my book.

Overall, your solutions are logical and well thought out. Your approach to each function tells me that you two were always thinking in the right direction.

Most of my feedback really dives into the details of each line. There are many comments about variable names and getting rid of extra lines of code. These pieces of feedback are to help you think more about "what's being used," "what do I need," etc., and to give you more permission to delete code or rename or refactor things. I hope my comments deepen your understanding and help inspire future projects. I hope that some of my suggestions help you think, "Oh! That's a cool refactoring suggestion," and I hope they prompt any questions that you can Slack DM me.

In addition, I'd like to comment that y'alls git hygiene is great, keep it up. My personal preference is to not include "wave 5 passed" etc in the commit messages, and to keep focusing on "what changes does this commit contain." Also, y'alls team reflections are great, too.

Overall, you two did a great job!

[tildeee]

It feels like there are two competing solutions in this function: new_movie is a variable that's never used, and you're validating the parameters twice. Consider this code, which passes the tests:

    if title is None or genre is None or rating is None:
        return None
    return {
            "title": title,
            "genre": genre,
            "rating": rating
    }

There are a lot of ways to write the same thing, so feel free to consider other options, but it would be good to watch for unused variables.

[tildeee]

[nit] Maybe y'all added the () around user_data["watched"] to help readability, but in the end, it feels like it doesn't conform with more general Python code style. I'd recommend getting rid of it:

    user_data["watched"].append(movie)

[tildeee]

In this line, you all re-assign the value of user_data["watchlist"] to watchlist. However, the original value of watchlist is a reference to user_data.get("watchlist", []). Therefore, consider the following code:

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

This code passes the tests! Again, when we .append() to a reference to a list (watchlist aka user_data.get("watchlist", [])), it will affect it everywhere, so we don't need to re-assign user_data["watchlist"] back to watchlist (aka we don't need user_data["watchlist"] = watchlist)

Let me know if you have questions!

[tildeee]

In this function, index is only being used within the for loop below. In this situation, we don't need to set index = 0 before the for loop-- for loops will make an index and use that! We can delete this line and the tests still pass.

[tildeee]

[nit] I'd encourage y'all to style this as user_data["watched"].append(movie)

[tildeee]

When this line is inside of the for loop, it runs every time. However, we only need this to run once-- after we've populated friends_watched_list fully. We can move this line after the nested for loops, and the tests still pass :)

    for friend in user_data["friends"]:
        for movie in friend["watched"]:
            friends_watched_list.append(movie["title"])

    friends_watched_set = set(friends_watched_list)

[tildeee]

In my eyes, friends_list isn't the most accurate name for this variable. I might propose unique_movies_list? Consider how this reads:

def get_unique_watched(user_data):

    friends_watched_list = []
    friends_watched_set = None
    unique_movies_list = []

    # other stuff...

    for movie in user_data["watched"]:
        if movie["title"] not in friends_watched_set:
            if movie not in unique_movies_list:
                unique_movies_list.append(movie)

    return unique_movies_list

[tildeee]

I love this line! It's very dense and compact.

[tildeee]

This is a re-implementation of a function you've alreaady made! :O :P :D Consider this code, where you invoke the other function and get to delete ten lines! And the tests still pass :)

def get_new_rec_by_genre(user_data):

    user_watched = set([movie["title"] for movie in user_data["watched"]])
    friends_unique_watched = get_friends_unique_watched(user_data)

    best_genre = get_most_watched_genre(user_data)

    recommended_movies = []
    # ...

[tildeee]

Here, your code checks if user_data has friends. If the user has friends, then it iterates through the unseen_by_friends list. If the user doesn't have friends, then the function should return an empty list, as there are no recommendations.

However, when we iterate with a for loop over an empty list, the code shouldn't break. There's a way to get rid of this if/else, but it would require fixing a small bug in get_unique_watched. Consider the following:

Imagine that get_unique_watched returned an empty list. If unseen_by_friends is an empty list, then when we do for movie in unseen_by_friends, it will skip over this loop, and recommended_movies will be an empty list. Therefore, with the fix in get_unique_watched, this function could look like this:

def get_rec_from_favorites(user_data):
    
    user_favorites = user_data["favorites"]
    recommended_movies = []

    unseen_by_friends = get_unique_watched(user_data)
    for movie in unseen_by_friends:
        if movie in user_favorites:
            recommended_movies.append(movie)
    return recommended_movies

I got this version of the code to work if, inside get_unique_watched, I moved the line friends_watched_set = set(friends_watched_list). Refer to that comment for details!

The reason why I like this refactoring is because we get to reduce the amount of validation, and we don't need to repeat it over several functions. Let me know what questions come up!