Ada AC2 Ocelots Class - Elaine(SiOk) Sohn

[sioksohn]

No description provided.

[kelsey-steven-ada]

🎉 Congrats on completing your first project at Ada! 🎉 I’ve added some suggestions & questions, please let me know if there's anything I can clarify.

[kelsey-steven-ada]

Nice assertions for all the relevant movie data!

[kelsey-steven-ada]

Great use of movie_to_watch to confirm the object we set up in the # Arrange step is in the watched list and was removed from watchlist!

[kelsey-steven-ada]

Nice handling to return None for missing data! In Python, when we test if a variable is pointing to None the PEP8 style guide recommends using is or is not for comparison rather than operators like ==.

None is a special object in Python, it’s a singleton, meaning there is only ever one instance of None in memory. All variables holding None are actually pointing to the same instance. By using is to compare against None, we avoid issues that could be caused by a class’s custom implementation of the comparison operators, and we gain some speed over a check using == since we are checking against identity rather than looking up if the variable's held type has a comparison function that should be used.

if title is None or genre is None or rating is None :
    return None

If you’d like more info on is vs == for None, here’s a couple resources to get started =]
https://peps.python.org/pep-0008/#programming-recommendations
http://jaredgrubb.blogspot.com/2009/04/python-is-none-vs-none.html

Something else to consider: here we're explicitly checking if the values are None, but what if we got an empty string for movie_title? If we wanted to check for empty strings at the same time as we check for None, we could rely on the truthy/falsy values of the parameters and write something like:

if not title or not genre or not rating:
    return None

[kelsey-steven-ada]

This function works, but it contains an "anti-pattern" that is 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 element is the movie that we are moving to user_data["watched"], during the second iteration through the loop it will throw an IndexError, because we will try to access the 2nd element in user_data["watchlist"], but there no longer is a second element.

This is a coding pattern that I recommend avoiding because it has so much potential for unforeseen errors. One fix is to return immediately after making the swap (ie, add return user_data 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.

[kelsey-steven-ada]

I would consider removing count. We can build up that value over the loop, but we can get that same value by calling len(user_data["watched"]).

[kelsey-steven-ada]

Great use of a frequency map genres to count the genre occurrences! Another way we could get the maximum value out of genres is with the max function:

max_value = max(genres, key=genres.get)
return max_value

[kelsey-steven-ada]

Really neat approach using sets!

Something to think about, here we will create 3 sets and a list, and we will iterate over user_data["watched"] twice and all data in user_data["friends"] once.

If we only create a set of the friend's titles, we can get the benefits of quick look up using a set, and make a single pass over user_data["watched"].

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

unique_movies = []
for movie in user_data["watched"]:
    if movie["title"] not in friends_movie_set:
        unique_movies.append(movie)

return unique_movies

[kelsey-steven-ada]

It looks like the indentation is too deep in this for loop, you might have indented twice.

[kelsey-steven-ada]

Love to see the code reuse!

This implementation will call get_friends_unique_watched in every iteration of the loop. Since our input data, and thus the result of calling get_friends_unique_watched, shouldn't be changing, we could reduce the processing time of our function by creating a variable that holds the result of get_friends_unique_watched before the loop starts. Because get_friends_unique_watched loops over our friend data already and provides us a list of movies that our user hasn't seen, we could also remove the for loops through the friend data here:

unique_friend_movies = get_friends_unique_watched(user_data)
recommended = []

for movie in unique_friend_movies:
    if movie['host'] in user_data['subscriptions']:
        recommended.append(movie)

return recommended

[kelsey-steven-ada]

We're nesting loops a little more than required here. We could remove the loop over user_data["favorites"] and keep the loop over the result of get_unique_watched. Then in each iteration, we'd check if the current move is in user_data['favorites'].

for uniq_movie in get_unique_watched(user_data):
    if uniq_movie in user_data['favorites']:
        rec_movies.append(uniq_movie)

return rec_movies