Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C19 Sapphire, Lou Loera and Linh Huynh #62

Open
wants to merge 28 commits into
base: main
Choose a base branch
from
Open

Conversation

mlhuynh
Copy link

@mlhuynh mlhuynh commented Mar 31, 2023

No description provided.

remitly-linh and others added 28 commits March 27, 2023 12:07
…d watch_movie functions in the party.py. Updated the assertion statement in the last two tests of test_wave_01.py.
… get_most_watched_genre, and get_available_recs.
…py and gave consistent spacing to docstring and comments
…py and gave consistent spacing to docstring and comments, Finishes from last commit
…by_genre and get_rec_from_favorites in party.py.
…ts for the following functions in party.py: create_movie, add_to_watched, add_to_watchlist, watch_movie, get_watched_avg_rating, and get_most_watched_genre.
…cy. Also added comments for the following functions: get_new_rec_by_genre, get_available_recs, and get_rec_from_favorites.
@mlhuynh mlhuynh changed the title C19 Sapphire, Linh Huynh and Lou Loera C19 Sapphire, Lou Loera and Linh Huynh Mar 31, 2023
Copy link

@tildeee tildeee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work on this project, Lou and Linh! This project gets a glowing "green" from me.

Overall, this code was a delight to read. It's apparent to me that you two like having a clean, orderly, straightforward, readable code. I think you all made great and wise decisions about when to use list/dict comprehensions-- it's never too much where it feels excessive or unreadable. Whenever you made local variables, even if it was only used one time, it's because it would greatly improve the readability. The code is so self-explanatory, if this were an industry pull-request, I'd probably request that you delete the comments and doc strings :P :D

Additionally, your git hygiene is great together. I think your commit messages are great and informative, too. Keep it up, I wouldn't ask for any change on that! Lastly, y'alls team contract and reflections are great.

Again, great work overall. Let me know if you have any questions.

# If so, return a dictionary named movie_details containing these 3 attributes as key-value pairs.
# If all 3 parameters are falsy, return None.

if title and genre and rating:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great way to use the truthiness/falsiness of these values for your condition. I like your decision to do this over if title != None and genre != None ...

ratings = []
for movie in user_data["watched"]:
ratings.append(movie["rating"])
return statistics.mean(ratings)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIIIIICE I love it! By using the statics package, this function reads so cleanly.

movie_genre_ranking[movie['genre']] += 1

sorted_genre_ranking = dict(sorted(movie_genre_ranking.items(),key=lambda item:item[1], reverse = True))
return list(sorted_genre_ranking.keys())[0]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done!

# Created a dictionay with the movie the user has watched
# Key : str / movie tiitle Value: dictionary containing movie info

watched_movies_by_user = {movie['title']:movie for movie in user_data['watched']}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done

for friend in user_data["friends"]:
for movie in friend["watched"]:
if movie['title'] in watched_movies_by_user:
del watched_movies_by_user[movie['title']]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using del is a common, popular, good way to remove an item from a dictionary in Python. However, in case you'd ever want to do this without using a keyword, I want to offer an alternative: pop. This line could be watched_movies_by_user.pop(movie["title"]).

(Again, I don't think you need to change this line now or in the future, I just wanted to give you an FYI in case you didn't already see pop)


movies_only_user_watched = []
for title in watched_movies_by_user:
movies_only_user_watched.append(watched_movies_by_user[title])
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's very clever that y'all put movie["title"]: movie data as the key-value pairs, so yo ucan get the movie data at the end here. Cool approach and solution :)

# Created a dictionay with the movie the users friends have watched
# Key : str / movie tiitle Value: dictionary containing movie info

movies_friends_watched = {}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Y'all picked variable names that are easy for me to understand :D and I'm very grateful.

@@ -158,13 +158,14 @@ def test_moves_movie_from_watchlist_to_empty_watched():
# Assert
assert len(updated_data["watchlist"]) == 0
assert len(updated_data["watched"]) == 1

raise Exception("Test needs to be completed.")
assert MOVIE_TITLE_1 in janes_data["watched"][-1]['title']
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this situation, since you're testing the value of strings, it might make sense to use == instead of in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants