Conversation
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Project one done!
There were a few waves with broken tests (after correcting for the syntax error that was preventing them from running). I've included minimal fixes that you can review, as well as slightly more modified versions incorporating some of my other feedback. Take a look!
The overall intent of your approaches was clear and easy to follow. This also made it easier for me to provide the corrections, so thank you for having things well-organized! The main thing that I'd recommend for those approaches would be to consider incorporating sets or dictionaries to improve some of the in lookups (especially when building unique results).
You did a great job building up data structures for the genre and ratings calculations, so now I'd recommend digging into the python library a bit more and see what methods exist to help us with these kinds of tasks.
Overall, nice job.
| @@ -0,0 +1,158 @@ | |||
| def create_movie(movie_title, genre, rating): | |||
| create_movie_dict = {'title': movie_title, 'genre': genre, 'rating': rating} | |||
| def create_movie(movie_title, genre, rating): | ||
| create_movie_dict = {'title': movie_title, 'genre': genre, 'rating': rating} | ||
|
|
||
| if movie_title and genre and rating: | ||
| movie_dict = {'title':movie_title, 'genre':genre, 'rating':rating} | ||
| return movie_dict | ||
| else: | ||
| return None |
There was a problem hiding this comment.
I'd suggest flipping the sense of the condition around to handle the special case (something is falsy) quickly and get out, rather than having that dangling else at the end. This arrangement is referred to as a guard clause or a guard condition. Notice that it lets us move the main focus of the function (creating the new dict) out of an indented block, letting it stand out more clearly.
def create_movie(movie_title, genre, rating):
if not movie_title or not genre or not rating:
return None
movie_dict = {'title':movie_title, 'genre':genre, 'rating':rating}
return movie_dict| return None | ||
|
|
||
|
|
||
| def add_to_watched(user_data, movie_title): |
| return user_data | ||
|
|
||
|
|
||
| def add_to_watchlist(user_data, movie_title): |
| for movie in movies_to_watch: | ||
| if movie_title == movie['title']: | ||
| movies_watched.append(movie) | ||
| movies_to_watch.remove(movie) |
There was a problem hiding this comment.
It's a little dangerous to modify a collection while we are iterating over it (here, the watchlist). In this case, it ends up being OK, since we really only expect a single copy of a movie to be in the watchlist at a time. However, if we were iterating over a list, and there was the possibility of multiple adjacent items being in the list that we want to remove, we could end up skipping over items as the list items get shifted around underneath the iteration.
If we know for certain that we only need to remove one thing, we could break out of the loop as soon as we've removed the one thing. Alternatively, we could find the movie record, and then wait to perform the remove until after completing that loop.
Again, nothing goes wrong in the current situation, but in general, we should be wary of modifying a collection while we are iterating through it.
|
|
||
| for friend in user_data["friends"]: | ||
| for movie in friend["watched"]: | ||
| friends_watched_list .append(movie) |
There was a problem hiding this comment.
Watch out for stray spaces like this. Python doesn't really care, but it looks a little odd.
| friends_watched_list .append(movie) | |
| friends_watched_list.append(movie) |
| if movie["title"] not in user_title: | ||
|
|
||
| if movie["host"] in host: | ||
| if movie not in rec_list: |
There was a problem hiding this comment.
These 3 conditions can all be combined with and
| for movie in user_data["friends"]: | ||
| for i in movie["watched"]: | ||
| new_list.append(i) |
There was a problem hiding this comment.
We need to guard against duplicates in the friends list here. Here's a minimal working change:
| for movie in user_data["friends"]: | |
| for i in movie["watched"]: | |
| new_list.append(i) | |
| for movie in user_data["friends"]: | |
| for i in movie["watched"]: | |
| if i not in new_list: | |
| new_list.append(i) |
And with more descriptive variable names:
for friend in user_data["friends"]:
for movie in friend["watched"]:
if movie not in new_list:
new_list.append(movie)As in a few situations above, we could make a set to track the added result titles in order to avoid the O(n) list in lookup.
| for movie in user_data["friends"]: | ||
| for i in movie["watched"]: | ||
| new_list.append(i) | ||
| for movie in new_list: |
There was a problem hiding this comment.
Do we have a function already that we could use to lookup the unique friend movies instead of re-implementing the logic here?
| def get_rec_from_favorites(user_data): | ||
| recs_from_faves = [] | ||
| unique_watched_list = get_unique_watched(user_data) | ||
| faves = user_data["favorites"] |
There was a problem hiding this comment.
If we convert this to a set of titles, we could avoid the O(n) in lookup on line 134.
2 participants
No description provided.