Tigers - Paje Bishop #158
Tigers - Paje Bishop #158
Conversation
There was a problem hiding this comment.
Good Work!
Even though I added several comments about a few small issues with style and test design as well as a bug in your code that our tests sadly miss, your implementation is both working (except for the bug) and clear to read. Thus this is good enough for a Green!
| # ******************************************************************************************* | ||
| # ****** Add assertions here to test that the correct movie was added to "watched" ********** | ||
| # ******************************************************************************************* | ||
| assert janes_data["watched"][0]["genre"] == GENRE_1 |
There was a problem hiding this comment.
This assertion is fine as a quick check, but definitely keep in mind that you're not limited to one assert here, and being through in your testing can have benefits. For example, I would at least add assertions to check that the "title" and "rating" keys are correct too.
| # ******************************************************************************************* | ||
| # ****** Add assertions here to test that the correct movie was added to "watched" ********** | ||
| # ******************************************************************************************* | ||
| assert updated_data["watched"][1] == HORROR_1 |
There was a problem hiding this comment.
Comparatively, this assertion is better because we're comparing the whole dictionary in updated_data["watched"][1] to HORROR_1, so all keys will be compared.
I personally would also check that the right movie was removed from updated_data["watchlist"], but as our instructions don't mention that, not really a miss on your part.
| @@ -54,13 +54,10 @@ def test_friends_unique_movies_not_duplicated(): | |||
|
|
|||
| # Assert | |||
| assert len(friends_unique_movies) == 3 | |||
|
|
|||
| assert friends_unique_movies.count(INTRIGUE_3) == 1 | |||
There was a problem hiding this comment.
Interestingly enough, I actually think this assertion here is fine even though it doesn't check every movie in the list. The main goal of this test is to check that movies don't get duplicated more than the entire list is correct as per our instructions.
Good test design!
|
|
||
| def add_to_watched(user_data, movie): | ||
| user_data["watched"] = [movie] | ||
| return user_data |
There was a problem hiding this comment.
Ah, this is kind of a mistake on our tests but there is a bug here that they don't catch.
Line 9 here will replace the entire value of user_data["watched"]. That's fine if there are no entries in the list, but if there had been entries in the list, we'd lose them in favor of the new list with one entry.
Not too major and sorry for our tests missing it. That being said, usually the expected behavior of adding to a list is to append the new element.
There was a problem hiding this comment.
Plus you otherwise seem to know how to use append (for example in the below function) so not major enough an issue to possibly move down to Yellow or Green.
|
|
||
| def add_to_watchlist(user_data, movie): | ||
| user_data["watchlist"] = [movie] | ||
| return user_data |
There was a problem hiding this comment.
The same bug as add_to_watched applies here too. But as our tests didn't catch it, that's on us more than you.
| for movies in user_data["watched"]: | ||
| genres.append(movies["genre"]) | ||
| if genres: | ||
| most_watched_genre = max(genres, key = genres.count) |
There was a problem hiding this comment.
This works, but there's a minor performance hit. max is an O(n) function and so is the count method. In this case max will end up calling count on every element in genres, bringing the whole complexity up to O(n^2). But there are ways to solve this in O(n) time, for example, using a frequency dictionary.
Now, as we hadn't even covered time complexity when you wrote this, this is not a major enough problem that I would move away from a Green. But I am pointing it out!
| def get_unique_watched(user_data): | ||
| unique_watched = [] | ||
| friends_watched = [] | ||
| for i in range(len(user_data["friends"])): |
There was a problem hiding this comment.
Nothing wrong with using range here, but this could read a little cleaner not using it:
for friend in user_data["friends"]:
for item in friend["watched"]:
friends_watched.append(item)
Mostly just a minor style nitpick, though. The same goes for a few other loops here and in get_friends_unique_watched but I won't belabor the point.
| for movie in friends_unique_watched: | ||
| if movie["host"] in user_data["subscriptions"]: | ||
| reccommendations.append(movie) | ||
| print(reccommendations) |
There was a problem hiding this comment.
I'm guessing this print statement was for debugging purposes. Not major, but usually good style to make sure to remove these after you're done debugging.
| if movie["genre"] == favorite_genre: | ||
| rec_list_by_genre.append(movie) | ||
| else: | ||
| return None |
There was a problem hiding this comment.
Looking at our instructions, I'm reading them as we were expecting that an empty list would be returned in this case, but it's completely legitimate that one would return None too!
However not really a fault on your part because it seems we didn't test this case in our tests either!
No description provided.