Ruby C19 - Bella A #46
Conversation
mmcknett-ada
left a comment
mmcknett-ada
left a comment
There was a problem hiding this comment.
Great job! Green 🟢
Overall comments:
- Great job implementing the new test cases
- Think about the difference in big-O time complexity for functions that used lists vs similar functions that used sets.
- Your code is well-formatted and easy to read!
| movies_info['title'] = title | ||
| movies_info['genre'] = genre | ||
| movies_info['rating'] = rating |
There was a problem hiding this comment.
Rather than creating an empty dictionary, then assigning the keys, we can use dictionary literal syntax to create the dictionary all at once.
movie_dict = { 'title': title, 'genre': genre, 'rating': rating }
return movie_dictIt could even be done all on one line
return { 'title': title, 'genre': genre, 'rating': rating }| if not title or not genre or not rating: | ||
| return None |
There was a problem hiding this comment.
[opinion] I prefer to have this code run before using title, genre, or rating.
| if movie['title'] == title: | ||
| user_data['watchlist'].remove(movie) | ||
| user_data['watched'].append(movie) | ||
| # return user_data |
There was a problem hiding this comment.
It would actually make sense to uncomment this! 😄
[warning] Here we are iterating over the user's watchlist, and also modifying it (remove). We should 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, either with a break statement or by doing return user_data like you have here commented out. 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!
| if not user_data["watched"]: | ||
| return None | ||
| for movie_dict in user_data["watched"]: | ||
| genre_occur[movie_dict["genre"]] = genre_occur.get(movie_dict["genre"], 0) + 1 |
There was a problem hiding this comment.
[opinion] I like how concise this is.
| # ----------------------------------------- | ||
|
|
||
|
|
||
| def get_unique_watched(user_data): |
There was a problem hiding this comment.
[perf] I think this function is optimal, from the perspective of big-O time complexity. I wouldn't change anything, personally, but it's always worth looking at it and asking whether you like the performance tradeoff compared to a simpler-to-read but less optimal implementation. 😊
|
|
||
|
|
||
| def get_friends_unique_watched(user_data): | ||
| user_watched = [] |
There was a problem hiding this comment.
[perf] Here you used a list, but in get_unique_watched. How do you think the operations on user_watched here compare to the operations on it in get_unique_watched?
There was a problem hiding this comment.
my teammate didn't like me using a set, so I used a list, that was initially a reason, but in terms of time complexity, set might be faster.
| friends_unique_watched = friends_watched | ||
| return friends_unique_watched |
There was a problem hiding this comment.
[nit] could just be:
return friends_watched| def get_available_recs(user_data): | ||
| friends_unique_watched = get_friends_unique_watched(user_data) | ||
| recommended_list=[] | ||
| for dict in friends_unique_watched: |
There was a problem hiding this comment.
[naming] I prefer a descriptive name like movie instead of dict, like you did in other functions.
2 participants
No description provided.