Sapphire - Mel M. & Jen T. #50
Conversation
Co-authored-by: Jennifer Tam <JenniferWTam@users.noreply.github.com>
Co-authored-by: Jennifer Tam <JenniferWTam@users.noreply.github.com>
| if title is None or genre is None or rating is None: | ||
| return None | ||
| else: | ||
| movie["title"]= title |
There was a problem hiding this comment.
[nit] add spaces before =
| movie["title"]= title | |
| movie["title"] = title | |
| for movie in user_data["watchlist"]: | ||
| if title in movie["title"]: | ||
| add_to_watched(user_data, movie) | ||
| user_data["watchlist"].remove(movie) | ||
|
|
||
| return user_data |
There was a problem hiding this comment.
This is iterating over the user's watchlist and also modifying it (remove). It's a good idea to avoid modifying a list while iterating over it to avoid problems with the iteration. If we stop iterating as soon as we make a change, it would still be safe (though anyone else reading the code would need to convince themselves of this). However, depending on our data restrictions, this code could end up modifying the list multiple times if there is more than one movie with the same title.
To ensure that only one change is made, we could break out as soon as we find the movie and move it. Again, this is safe from an iteration perspective, but may require other team members to spend time convincing themselves of this. We might instead decide to think of this as 2 steps. 1. Find the movie to move by iterating through the list, and 2. actually move the move (outside the list). This might be more clear that there are no negative modifying while iterating shenanigans!
for movie in user_data["watchlist"]:
if movie["title"] == movie_to_watch:
user_data["watched"].append(movie)
user_data["watchlist"].remove(movie)
break
return user_data| if len(genre_list) == 0: | ||
| return None | ||
| else: | ||
| return(multimode(genre_list)[0]) |
There was a problem hiding this comment.
Nice job finding a library function that handles this for you. I also love that you built it the long way by hand, too! It seems like your manual version isn't quite working (at least when I uncomment it and run it). You might benefit from coming back to this later for practice. :)
| for movie in user_watched: | ||
| if movie not in friends_watched and movie not in unique_movies: | ||
| unique_movies.append(movie) |
There was a problem hiding this comment.
Consider what the big-O runtime of this loop might be. It turns out that using in on a list actually hides a loop, so as the combined friends list gets longer, the length of time to see whether the next movie is already in the list also takes longer. We could use a helper set of movie titles to help us do this check more efficiently. Each time we add a movie to the combined list, we could add its title to a set. Then, when deciding whether or not to add the next movie, we could check whether that movie's title is in the title set already. in on sets is much more efficient than on lists.
There was a problem hiding this comment.
It'd be a good idea to think about the time complexity of the other methods that use in, too.
No description provided.