Ocelots - Yvette W. #21
Conversation
| @@ -0,0 +1,13 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
Committing new files is no issue for this project, but something to think about going forward is that we generally don't want to commit changes to files that we don't intend to purposefully share with other folks. This can mean having to be more selective when staging files for commit over being able to use git add .
| @@ -1,10 +1,11 @@ | |||
| import pytest | |||
| # NOTE: In production code, we developers should change import * to something more specific. Due to some constraints of this project, we will import * in our test files. | |||
| # from viewing_party.main import * | |||
| from viewing_party.party import * | |||
There was a problem hiding this comment.
It looks like this line was updated to duplicate the line below it. Was there a something you ran into that required this to be changed?
| # ******************************************************************************************* | ||
| # ****** Add assertions here to test that the correct movie was added to "watched" ********** | ||
| # ******************************************************************************************* | ||
| assert updated_data["watched"][0] == MOVIE_TITLE_1 |
There was a problem hiding this comment.
In this case where the test is checking individual keys of a dictionary, I recommend including assertions for all of the relevant keys (title, rating, and genre).
If we take a look at the nested data that is the input to our function, each movie entry in the lists watchlist and watched should be a dictionary of the form:
{
"title": MOVIE_TITLE_1,
"genre": GENRE_1,
"rating": RATING_1
}The way this assertion is structured makes me concerned that the format of our data structure has fundamentally changed, which is something we want to avoid. We want to preserve the existing data formatting even as we move individual dictionary entries from the list watchlist to the list watched.
| # ******************************************************************************************* | ||
| # ****** Add assertions here to test that the correct movie was added to "watched" ********** | ||
| # ******************************************************************************************* | ||
| assert updated_data["watched"][1] == movie_to_watch["title"] |
There was a problem hiding this comment.
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"]| # ************************************************************************************************* | ||
| # ****** Add assertions here to test that the correct movies are in friends_unique_movies ********** | ||
| # ************************************************************************************************** | ||
| assert INTRIGUE_3 in friends_unique_movies |
There was a problem hiding this comment.
I recommend checking that all 3 titles we expect are present in the list friends_unique_movies.
| friends_watched.append(m) | ||
| # remove duplicates | ||
| friends_watched_nd = list() | ||
| [friends_watched_nd.append(i) for i in friends_watched if i not in friends_watched_nd] |
There was a problem hiding this comment.
We would generally want to restructure this statement as either a for loop:
friends_watched_nd = list()
for i in friends_watched:
if i not in friends_watched_nd:
friends_watched_nd.append(i)or as a single list comprehension:
friends_watched_nd = [i for i in friends_watched
if i not in friends_watched_nd]I also highly recommend choosing a variable name other than i. i is typically used to represent an integer counter in ranged for loops and could be misleading to readers. I would suggest a name that represents the data the variable points to. Since i is a dictionary that represents a movie, what is a name that better represents this data?
| friends_watched.append(m) | ||
| # remove duplicates | ||
| friends_watched_nd = list() | ||
| [friends_watched_nd.append(i) for i in friends_watched if i not in friends_watched_nd] |
There was a problem hiding this comment.
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. I suggest checking out the PEP8 resource for suggestions on how to break up a statement across lines when necessary: https://peps.python.org/pep-0008/#maximum-line-length.
| recs = list() | ||
| friends_watched = list() | ||
| for f in user_data["friends"]: | ||
| for m in f["watched"]: | ||
| friends_watched.append(m) | ||
| # remove duplicates | ||
| friends_watched_nd = list() | ||
| [friends_watched_nd.append(i) for i in friends_watched if i not in friends_watched_nd] | ||
|
|
||
| if (len(user_data["subscriptions"])!=0) and (len(friends_watched)!=0): | ||
| for i in friends_watched_nd: | ||
| if i["host"] in user_data["subscriptions"] and i not in user_data["watched"]: | ||
| recs.append(i) | ||
| return recs |
There was a problem hiding this comment.
It looks like we duplicate some of our prior logic here. Another option would be to reuse the get_friends_unique_watched function to get a list of movies our users has not seen, then filter that list by available subscriptions:
unique_friend_movies = get_friends_unique_watched(user_data)
recs = []
for movie in unique_friend_movies:
if movie['host'] in user_data['subscriptions']:
recs.append(movie)
return recs| genre = get_most_watched_genre(user_data) | ||
| unwatched = get_friends_unique_watched(user_data) |
| friends_watched = list() | ||
| for f in user_data["friends"]: | ||
| for m in f["watched"]: | ||
| if m not in friends_watched: | ||
| friends_watched.append(m) | ||
| watched = get_unique_watched(user_data) |
There was a problem hiding this comment.
get_unique_watched will give us a list of movies that we have watched but none of our friends have seen. If we looped over the result of get_unique_watched and in each iteration, checked if the current move is in user_data['favorites'] we could remove the code to fill friends_watched and check it later, because the list watched from get_unique_watched has already filtered out the movies our friends have watched.
watched = get_unique_watched(user_data)
recs = []
for movie in watched:
if movie in user_data['favorites']:
recs.append(movie)
return recs
No description provided.