Sapphire - Kathleen Shin #47
Conversation
…pty_watched function
There was a problem hiding this comment.
Tagging @erickalucia for visibility
Great work, Kathleen and Ericka! This project looks great and it's marked as a "green."
Overall, the code is well-written, and had the approach and solutions I was hoping for. I wish I had more feedback, but truly, the code was readable and good. Keep doing what you're doing!
Some of my comments are about small logic things, so let me know what questions come up from it.
In addition, your git hygiene is great. I'd recommend saying "Implemented" instead of "completed" because no code is ever truly complete >:P It seems like Ericka's account didn't get to push anything to this project, reach out to me if there's a Git issue that needs to be sorted out.
Also, your tests look great, and your team reflections look great!
Again, overall you two did a great job. Let me know if any questions come up.
| if not title: | ||
| return None | ||
| elif not genre: | ||
| return None | ||
| elif not rating: | ||
| return None | ||
| else: |
There was a problem hiding this comment.
Good if/elif/else conditionals! The line return None is repeated a few times. It might be nice to refactor and combine some of these conditional cases. DRY!
| genre_length = len(movie_genre_dict[genre]) | ||
| if genre_length >= max_count: | ||
| max_genre = genre | ||
| max_count = len(movie_genre_dict.values()) |
There was a problem hiding this comment.
Hmm... Looking through your logic, I see that if a genre's length is bigger than max_count, then this genre is the new max_genre and its length is the new max_count. I would expect this line to be:
max_count = len(movie_genre_dict[genre])This code also passes the tests!
| if len(friends_watched) == 0: | ||
| return rec_by_genre |
There was a problem hiding this comment.
Actually, we can get rid of this conditional and the tests still pass! If friends_watched is empty, then the code skips the below for loop, but still returns rec_by_genre (which is still an [])
| if len(user_data["watched"]) == 0: | ||
| return None |
There was a problem hiding this comment.
I've written a similar comment in get_new_rec_by_genre. Turns out, we can delete these two lines, and the tests still pass. If users_data["watched"] is empty, then it skips the for loop below, still sets max_genre to None. Then, because movie_genre_dict will be empty, it'll skip that for loop too... leading to return max_genre!
I'm emphasizing this point because it's helpful to trust that a for loop will not break if it iterates over an empty [] or {}. If we trust that, then we can save ourselves on writing extraneous code, and extraneous code always has potential to lead to more bugs.
| else: | ||
| user_data["watched"].append(movie) | ||
| user_data["watchlist"].remove(movie) | ||
| print (user_data) |
There was a problem hiding this comment.
[nit] Flagging a stray print statement that got left behind. Also, lines 20 and 29-32. Keep printing things and testing them! It'd be nice if they were deleted by submission time, or even before they're committed to git.
No description provided.