Phoniex_C22_Tram_Beenish #10
Conversation
| if not title or not genre or not rating: | ||
| return None | ||
| movie_dict = {} | ||
| if type(title) == str and type(genre) == str and type(rating) == int or type(rating) == float or int: |
There was a problem hiding this comment.
we noticed after we submitted but the or int at the end of the line is not needed.
| if type(title) == str and type(genre) == str and type(rating) == int or type(rating) == float or int: | |
| if type(title) == str and type(genre) == str and type(rating) == int or type(rating) == float: | |
There was a problem hiding this comment.
Thanks for pointing this out. Including a check like this (though not required by the project prompt) can be helpful, as it can identify bad data before it gets into our objects. If we didn't have this check, Bad data could sneak into an object at some point, only to cause problems later on. Then we'd have to try to trace back to where the bad data came from.
Intersting, the or int literally does nothing here. It might be tempting to read the type(rating) == float or int as checking that rating is either an int or a float, with the earlier separate check for int making the or int redundant, but that's not what's happening. Instead, with the previous rating checks, it's more like this
(type(rating) == int) or (type(rating) == float) or (int)and and or have lower precedence than the comparison operators, so they act to group together multiple entire comparisons, not multiple values within a comparison. So we have the two checks for the type of rating, anfd then or int off by itself. It turns out that if rating is an int or float, the the interpretation of int on its own doesn't matter. If the value on the left of or is truthy, it doesn't even try to evaluate the value on the right. But if the value on the elft is falsy, it does need to evaluate this. It turns out that all "types" (like int) are truthy, so this is like having or True stuck on the end. This has the effect of deactivating the float and int type checks, because even if rating is neither a float nor an int, the final or int will cause that check to pass.
So even more than not being needed, the or int check is causing a problem.
One thing to watch out for in a more general case is that explicit type() checks can sometimes be overly strict. It's often preferred to do an isinstance check, whcih takes into account type inheritance in Python. In other cases, especially if we're not just storing the value to use later, we might just try to use the value without checking the type at all. If it works, it works. If not, hopefully an error gets raised on the part that's unsupported. This tends to be the most flexible thing to do to remain compatible with python's lack of static type checking.
Statically typed languages like the C language family, Java, and many others, do away with needing to perform checks like this by explicitly declaring the valid types of the parameters as part of the function definition. THe compiler would catch an attempt to pass in the wrong kind of data before even running the code.
| # ******************************************************************************************* | ||
| # ****** Add assertions here to test that the correct movie was added to "watched" ********** | ||
| assert janes_data["watchlist"] == [] |
There was a problem hiding this comment.
We probably don't need this check for the empty list, since we have an earlier check that the length of the watchlist is 0. Technically, this also ensures that the type of the collection is still a list, but it would be very surprising if that had changed.
| assert janes_data["watched"] == [{ | ||
| "title": MOVIE_TITLE_1, | ||
| "genre": GENRE_1, | ||
| "rating": RATING_1 | ||
| }] |
There was a problem hiding this comment.
👍 Great approach for checking all three pieces of data of the dict are the expected values. By building a temporary dict with the keys and values, the value comparison of the dictionary will take care of the value by value comparison.
Notice that we can only definitively know the location of the movie in the watched list because there's only a single item in the list. The project made no specification about where in the list items should end up when moving to the watched list.
| assert janes_data["watchlist"] == [FANTASY_1] | ||
| assert janes_data["watched"] == [FANTASY_2, movie_to_watch] |
There was a problem hiding this comment.
👍 Great idea to check that the contents that should not have been affect by the move are still in the expected lists.
Here, rather than literally checking that the 2 movies in the watched list are in the order listed here, we might prefer to use a few in checks. While it's true that your implementation will put a recently watched movie at the end of the list, that behavior is not in the specification.
Checking for more strict behaviors than are specified can tie our hands down the road if we wanted to perform some kind of refactoring later on that didn't maintain this same guarantee.
| assert INTRIGUE_3 in friends_unique_movies | ||
| assert HORROR_1 in friends_unique_movies | ||
| assert FANTASY_4 in friends_unique_movies |
There was a problem hiding this comment.
👍 Great way to check that the three movies are in the output.
Here, it's even more important not to assume a particular ordering of the outputs. The act of making some data unique can often result in data ending up in arbitrary locations relative to where they occurred in the original data, especially if intermediate data structures like sets or dicts are involved.
| #Act (completed by us) | ||
| recommendations = get_new_rec_by_genre(sonyas_data) | ||
|
|
||
| raise Exception("Test needs to be completed.") | ||
| # ********************************************************************* | ||
| # ****** Complete the Act and Assert Portions of these tests ********** | ||
| # ********************************************************************* | ||
| # Assert(completed by us) | ||
| assert len(recommendations) == 0 |
There was a problem hiding this comment.
👍 This test needed both the act and the assert.
There was a problem hiding this comment.
Nit: leave a space after the # character in comments
# Act (completed by us)| recommendations = [] | ||
| for movies in friend_unique_movies: | ||
| if movies["host"] in user_data["subscriptions"]: | ||
| recommendations.append(movies) | ||
| return recommendations |
There was a problem hiding this comment.
Code like this is an ideal candidate for a list comprehension. A list comprehension is a shorthand syntax for creating a new list, based on processing/filtering another list. This logic could be rewritten as
return [movie for movie in friend_unique_movies if movie["host"] in user_data["subscriptions"]]The first time we see this, it may feel more complicated, but it's a common enough pattern that we should get used to seeing/writing it.
| def get_available_recs(user_data): | ||
| friend_unique_movies = get_friends_unique_watched(user_data) | ||
| recommendations = [] | ||
| for movies in friend_unique_movies: |
There was a problem hiding this comment.
Nit: The loop variable should usually be a singular noun (so movie not movies) since it refers to each element in the collection one by one.
| recs = get_friends_unique_watched(user_data) | ||
| new_rec_by_genre = [] | ||
| most_freq_genre = get_most_watched_genre(user_data) |
There was a problem hiding this comment.
👍 Another great reuse of previous wave functions!
| for movie in recs: | ||
| if movie["genre"] == most_freq_genre: | ||
| new_rec_by_genre.append(movie) | ||
| return new_rec_by_genre |
There was a problem hiding this comment.
Here's another great candidate for a list comprehension: the logic that builds a filtered list of movies to those sharing a genre with the most watched genre.
|
|
||
| def get_rec_from_favorites(user_data): | ||
| rec_from_favorites = [] | ||
| unique_watched = get_unique_watched(user_data) |
There was a problem hiding this comment.
👍 And here we can start from the unique list of user watched movies, then filter it down to those that also appear in the favorites list.
No description provided.