"SHARKS Raha MR" #102
"SHARKS Raha MR" #102
Conversation
There was a problem hiding this comment.
Great work Raha! You hit all the learning goals for this project and all tests are passing. You have earned a well-deserved 🟢 grade on this project 🦈✨
I added comments to your PR primarily on ways to refactor your code, improve tests, and improve the readability of your code. In the future, I'd like to see more docstrings in your functions. Docstrings are a great way of leaving notes to your future self about your approach to writing these functions.
In future projects, I'd like to see you use more for in loops when iterating through the values of a list and to use more descriptive variable names.
Feel free to refactor and improve on this project and do let me know if you want me to take another look for fun 😄
Keep up the great work! ✨
| assert updated_data["watched"][0]["title"] is MOVIE_TITLE_1 | ||
| assert updated_data["watched"][0]["genre"] is GENRE_1 |
There was a problem hiding this comment.
This is a much-needed change in our tests but our test asserts should use == instead of is.`
== checks for value equality while is checks for reference equality (if both references point to the same object in memory).
is is typically used when we check whether a value equates to None i.e. "if user_data is None".
There was a problem hiding this comment.
So in this case the assert should look like:
| assert updated_data["watched"][0]["title"] is MOVIE_TITLE_1 | |
| assert updated_data["watched"][0]["genre"] is GENRE_1 | |
| assert updated_data["watched"][0]["title"] == MOVIE_TITLE_1 | |
| assert updated_data["watched"][0]["genre"] == GENRE_1 |
| assert updated_data["watched"][1]is HORROR_1 | ||
| assert updated_data["watched"][0]is FANTASY_2 |
There was a problem hiding this comment.
It's also more pythonic to have spaces between expressions, keywords, and operators:
assert updated_data["watched"][1] == HORROR_1
assert updated_data["watched"][0] == FANTASY_2| assert INTRIGUE_3 in friends_unique_movies | ||
| assert HORROR_1 in friends_unique_movies | ||
| assert FANTASY_4 in friends_unique_movies | ||
| assert FANTASY_2 not in friends_unique_movies |
| # Act | ||
| recommendations = get_new_rec_by_genre(sonyas_data) | ||
|
|
||
| # Assert | ||
| assert len(recommendations) == 0 |
| movie_table = {} | ||
|
|
||
| if title and genre and rating: | ||
| movie_table["title"] = title | ||
| movie_table["genre"] = genre | ||
| movie_table["rating"] = rating | ||
| return movie_table |
There was a problem hiding this comment.
Great work Raha! I like how you added key-value pairs by first checking for truthiness.
Another way to write this function is by using a guard clause and building the dictionary directly in the return statement. This allows your function to exit early when falsy values are passed in and reduces the amount of nested code we have, allowing the primary purpose of the function to be slightly easier to read.
def create_movie(title, genre, rating):
if not title or not genre or not rating:
return None
return {
"title": title,
"genre": genre,
"rating": rating
}| favorite = Counter(genre) | ||
| fave_max = max(favorite, key=favorite.get) |
There was a problem hiding this comment.
The Counter collection is handy but we could use get_most_watched_genre() as a helper function.
| fave_max = max(favorite, key=favorite.get) | ||
| results = [] | ||
|
|
||
| for i in range(len(list(friends_watched[0]["watched"]))): |
There was a problem hiding this comment.
Loop variables should also be descriptive names. i is too generic. Having descriptive names improves readability and helps us understand our code when we revisit it in months or years later.
| return [] | ||
| favorites = user_data["favorites"] | ||
|
|
||
| all = [] |
There was a problem hiding this comment.
all can also be mistaken as the all() function. Perhaps all_movies_list or all_movies would be a good name for this scenario?
| for a in range(len(friends_watched)): | ||
| for b in range(len(friends_watched[a]["watched"])): | ||
| all.append(friends_watched[a]["watched"][b]["title"]) |
There was a problem hiding this comment.
How might this look with a for in loop/
| c = [x for x in favorites if x not in mutual] | ||
|
|
||
| return c |
There was a problem hiding this comment.
Great list comprehension but what does c represent?
No description provided.