Ocelots - Kelly T

[kellytate]

No description provided.

[kelsey-steven-ada]

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

[kelsey-steven-ada]

Nice assertion for the title!

Since we have the assertion above assert len(updated_data["watched"]) == 1 that tells us there is only 1 item in the watched list, we could also use updated_data["watched"][0] to access the movie we want to check.

In this case where the code is checking individual keys of a dictionary, I recommend including assertions for all of the relevant keys.

    watched_movie = updated_data["watched"][0]
    assert watched_movie["title”] == MOVIE_TITLE_1
    assert watched_movie["rating"] == RATING_1
    assert watched_movie["genre"] == GENRE_1

[kelsey-steven-ada]

The README isn't explicit about what order we should add movies to the list watched. If we want our tests to be independent from a specific implementation that adds movies to a particular location in the watched list, we could use the in keyword to check if the variable movie_to_watch is in the watched list instead of looking at a specific index. By checking for the whole dictionary, there's a side benefit that we don't need to check each key independently:

assert movie_to_watch in updated_data["watched"]

# Another option:
assert HORROR_1 in updated_data["watched"]

[kelsey-steven-ada]

Nice use of a list comprehension to create the set here!

This assertion will create a set from the movie titles in friends_unique_movies and check that the length of that set is the same as the length of friends_unique_movies. This will ensure we don't have duplicates, but it doesn't confirm that friends_unique_movies holds only the movies we expect. I would suggest also checking if the specific movies we intend to be in friends_unique_movies are present.

What could those assertions look like?

[kelsey-steven-ada]

The PEP 8 style guide recommends limiting lines to a max of 79 characters to make code easier to read, especially with multiple editor windows open: https://peps.python.org/pep-0008/#maximum-line-length. Another option here could be to create the set on a separate line, then use it in the assert:

title_set = set(movie["title"] for movie in friends_unique_movies)
assert len(title_set) == len(friends_unique_movies)

[kelsey-steven-ada]

Great act & assert steps!

[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 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]

Nice use of set!

[kelsey-steven-ada]

The PEP 8 style guide recommends limiting lines to a max of 79 characters to make code easier to read, especially with multiple editor windows open: https://peps.python.org/pep-0008/#maximum-line-length. We would want to break it over a couple lines or create variables to hold the comparisons that we then use in an if statement:

is_movie_in_my_list = movie_entry in user_data["watched"]
is_movie_in_unique_list = movie_entry in friends_unique_movies
if not is_movie_in_my_list and not is_movie_in_unique_list:
    friends_unique_movies.append(movie_entry)

[kelsey-steven-ada]

Nice code reuse!

[kelsey-steven-ada]

Great place for a list comprehension! I would suggest breaking this over a couple lines:

recommendations = [movie for movie in possible_recs 
                  if movie["host"] in user_data["subscriptions"]]

[kelsey-steven-ada]

Though this solution passes the current test suite, it isn't doing what the problem statement asks.

The test test_new_genre_rec uses clean_wave_5_data() as the input, which includes subscription data. If we removed "hulu" from the user's subscriptions on line 157 of test_constants.py, suddenly the test test_new_genre_rec starts failing. Our problem statement for get_new_rec_by_genre does not ask us to consider subscriptions though, so changing that subscription data should not change the result of get_new_rec_by_genre.

What steps can you take to diagnose what is happening?
How do we make sure that we are only considering movies where the genre of the movie is the same as the user's most watched genre?

[kellytate]

I misread the instructions (read 'genres' instead of 'genre') and made a flawed assumption that the get_new_rec_by_genre function should build on the previous function and require matching subscription. First troubleshooting step should have been to reread the problem statement carefully. Doing so revealed that I needed to check the possible_recs against the result of the get_most_watched_genre function.

[kelsey-steven-ada]

Nice investigation and updates!