Sapphire - Niambi P. #64
Conversation
…ote code in party.py
…empty_user_watched
…watchlist in Wave 1.
…from_watchlist_to_empty_watched
…_if_movie_not_in_watchlist
…s function in party.py
…_nothing_if_movie_not_in_watchlist
…calculates_watched_avg_rating
…mpty_watched_average_rating_is_zero.
…enre_is_None_if_empty_watched in wave 2
…, test_my not_unique_movies
…in Wave 3, also defined helper function get_friends_movie_titles
…cated in wave_03 tests & passed test_friends_not_unique_movies.
… pass test_new_genre_rec_from_empty_watched
…ed remaining tests
…_from_favorites, test_uique_from_empty_favorites, and test_new_rec_from_empty_friends
|
Sapphire - Hannah Chandley Wohllaib also worked on this project. Another note: when using live share in VS code we can only commit from the machine of whoever is sharing, but we did take turns writing code and doing commits and pushes during this time even though for periods it looks like just one of us. Once we realized this we tried to switch who was live sharing more and then stopped using live share. |
There was a problem hiding this comment.
Tagging @Lemmi-C for visibility
Nice work on this project, y'all! The project is marked as a "green" for me.
Overall, the code is great. I followed the logic, it's very readable, and it hits all of the requirements. It's very apparent to me that you two practiced iteration and accessing dictionaries and lists :) Most of my comments are around different lines of code that I would continue to refactor or think about. I hope the feedback carries with you into future projects, or you ask me any questions that come up :D
Also, your git hygiene is great! Keep up the good, descriptive commit messages and commit frequency. In addition, the tests looked good, and both of your team reflections looked great and I appreciate them.
Again, good work on the project, and keep it up!
| if title == None or genre == None or rating == None: | ||
| new_movie = None | ||
| else: | ||
| new_movie = {"title": title, "genre": genre, "rating": rating} | ||
| return new_movie |
| def add_to_watched(user_data, movie): | ||
| watched_list = user_data["watched"] | ||
| watched_list.append(movie) | ||
| user_data["watched"] = watched_list |
There was a problem hiding this comment.
If we delete/comment this line out, we can actually see that the tests still pass-- we don't need to re-assign user_data["watched"] = watched_list. That's because watched_list is a reference to user_data["watched"]-- anything we change on watched_list will change on user_data["watched"], because they're both pointing to the same thing. Let me know if you have questions on this!
| def add_to_watchlist(user_data, movie): | ||
| watchlist = user_data["watchlist"] | ||
| watchlist.append(movie) | ||
| user_data["watchlist"] = watchlist |
There was a problem hiding this comment.
Similar to above, watchlist and user_data["watchlist"] refer/point to the same thing, so we don't need to re-assign watchlist back.
| tracker_dict = None | ||
| for movie_dict in user_data["watchlist"]: | ||
| if movie == movie_dict["title"]: | ||
| add_to_watched(user_data, movie_dict) |
There was a problem hiding this comment.
Great decision to use the add_to_watched function here! A lot of people didn't think to do this, but I think it's ideal code to reuse it.
| for i in range(len(user_data["watched"])): | ||
| sum += user_data["watched"][i]["rating"] |
There was a problem hiding this comment.
In this situation, I think it's better to use a for loop on each item in user_data["watched"], rather than i over a range. I think that because we use i to access the user_data. I'd choose to use i if we needed to use it as a number.
Consider the following refactor, which passes the tests!
for movie in user_data["watched"]:
sum += movie["rating"]| for movie in user_data["watched"]: | ||
| user_movie_titles.add(movie["title"]) | ||
|
|
||
| friends_unique = friends_movie_titles - user_movie_titles |
There was a problem hiding this comment.
Nice way to find the difference between them! I like the use of sets, too.
| for friend in user_data["friends"]: | ||
| for movie in friend["watched"]: | ||
| if movie["title"] in friends_unique: | ||
| if movie not in friends_unique_movies: | ||
| friends_unique_movies.append(movie) |
There was a problem hiding this comment.
Your code is correctly indented. However, the loops and conditionals are getting pretty deeply nested here-- We can't avoid every nested loop, but when I am 3+ levels in, I'd look for a way to refactor the code. As a starting point, I would try to combine the most inner ifs.
| if "subscriptions" in user_data: | ||
| user_subs = user_data["subscriptions"] | ||
| recs = [] | ||
|
|
||
| for movie in friends_watched: | ||
| if movie["host"] in user_subs: | ||
| recs.append(movie) | ||
|
|
||
| return recs | ||
|
|
||
| else: | ||
| return [] |
There was a problem hiding this comment.
Your function logic is correct here. If you had time, I would encourage thinking about how to refactor this code.
I'm pretty enthusiastic about refactoring, so I'll describe my own process of refactoring here:
First, I notice three interesting lines:recs = [], return recs, and return []. These three lines stand out to me because they all feel kind of similar. I think about how return [] is the same as return recs if recs is an empty array, which it starts off as!
Then, I'd just try to change small pieces of code. I'd replace the return in the else clause with return recs.
After that, seeing that the line return recs is repeated, I'd delete the else, move a return recs line outside of the conditional, and delete all other return recs
Then, I'd move my definition of recs (the line recs = []) to the beginning of the function, so it is on the same indentation as when we return it.
Consider this, which passes the tests:
def get_available_recs(user_data):
friends_watched = get_friends_unique_watched(user_data)
recs = []
if "subscriptions" in user_data:
user_subs = user_data["subscriptions"]
for movie in friends_watched:
if movie["host"] in user_subs:
recs.append(movie)
return recsThis refactored code uses only code you two wrote, but by moving a variable definition, we can get rid of the whole else clause.
PS: Sorry if I'm too enthusiastic or nerdy about this and wrote too much, I only hope it's helpful!
| for favorite in favorites: | ||
| if favorite not in friends_watchlist: | ||
| recs.append(favorite) | ||
| return recs |
No description provided.