Sapphire - Anna Lima #65
Conversation
git commit -m 'wave 01 success!' q exit /
Edith worked on this code as you can see from the commits :) |
There was a problem hiding this comment.
Well done on this project, you two! This project is marked as a "green."
Overall, your project code is looking great. It's straightforward and good logic. To be honest, the code is readable and straightforward enough, I think that your code may speak for itself-- if you felt like deleting some if your comments, I think your code would still look good. My feedback is mostly around small, optional opportunities for refactoring.
Also, your tests look good! Your team contract and team reflections also look good!
Additionally, your git hygiene is pretty good. However, I would expect the commit messages to generally start with a verb and describe the code changes in the commit (and less "Wave 2 passes" etc). Sometimes merge commits simply say "merge" though, so you all were right. The commit frequency is what I would expect and look for though :) Edith, if your git is still in a weird state, let me know and we can try to get it fixed together!
Again, the project is great overall. Good work, keep it up.
| if not title: | ||
| return None | ||
| if not genre: | ||
| return None | ||
| if not rating: | ||
| return None |
There was a problem hiding this comment.
The line return None is repeated here a few times. This code is readable and concise as-is. But for optional refactoring, maybe we can make this more DRY
| genre_dict[movie_genre] += 1 | ||
|
|
||
| #return genre with highest value from genre_dict | ||
| most_popular_genre = max(genre_dict, key=genre_dict.get) |
There was a problem hiding this comment.
Fantastic use of max :D It helped shave a whole loop here.
| for movie in friends_watched_movies: | ||
| friend_movie_list += movie["watched"] |
There was a problem hiding this comment.
In my opinion, more appropriate variable names might be:
friends = user_data['friends']
friend_movie_list = []
for friend in friends:
friend_movie_list += friend["watched"]| for movie in friend_movie_list: | ||
| movie_host = movie['host'] | ||
| #if the movie host is also in the subscriptions that the user has; the user hasn't watched the movie yet | ||
| if movie_host in service_subscriptions and movie not in user_watched_movies and movie not in recs: |
There was a problem hiding this comment.
In this situation, since movie_host is only used once, I find it fairly readable to refactor this to:
for movie in friend_movie_list:
#if the movie host is also in the subscriptions that the user has; the user hasn't watched the movie yet
if movie['host'] in service_subscriptions and movie not in user_watched_movies and movie not in recs:The tests still pass with this change, too!
No description provided.