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

Franklyn Rod. Snow Leopards #145

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

FranklynRod
Copy link

No description provided.

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

Choose a reason for hiding this comment

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

Good job here! We could also test for the other key/values in the dictionary:

assert updated_data["watched"][0]["rating"] == RATING_1
assert updated_data["watched"][0]["genre"] == GENRE_1

@@ -142,13 +143,15 @@ def test_moves_movie_from_watchlist_to_watched():
# Assert
assert len(updated_data["watchlist"]) == 1
assert len(updated_data["watched"]) == 2
assert updated_data["watched"][1]["title"] == MOVIE_TITLE_1

Choose a reason for hiding this comment

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

This works well but you can also test to see if the movie is in the watched list as well as some other assertions to ensure no unwanted list modifications have occurred:

    assert movie_to_watch in updated_data["watched"]
    assert FANTASY_2 in updated_data["watched"]
    assert FANTASY_1 in updated_data["watchlist"]
    assert movie_to_watch not in updated_data["watchlist"]


raise Exception("Test needs to be completed.")
assert HORROR_1 in friends_unique_movies
assert FANTASY_4 in friends_unique_movies

Choose a reason for hiding this comment

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

Good job here! You could also add the assertion to check for the 3rd movie as well.


raise Exception("Test needs to be completed.")
# Assert
assert rec_from_empty_friends == []

Choose a reason for hiding this comment

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

Nicely done 👍

def create_movie(title, genre, rating):
pass
if not title or not genre or not rating:

Choose a reason for hiding this comment

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

Excellent use of the guard clause pattern! Early return here means no unnecessary code execution!

new_dict["title"] = title
new_dict["genre"] = genre
new_dict["rating"] = rating
# if not new_dict["key"]

Choose a reason for hiding this comment

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

It's generally not a great practice to leave commented out code in Pull Requests. Also, you can return a dictionary literal like so, rather than creating an empty dict and adding key/values one by one:

return {
    "title": title,
    "rating": rating,
    "genre": genre
}

for movie in user_data["watchlist"]:
if movie["title"] == title:
user_data["watched"].append(movie)
i = user_data["watchlist"].index(movie)

Choose a reason for hiding this comment

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

This works just fine, but we can use remove with the item since we know the item exists (since we're iterating over them directly). If we're worried about modifying the list we're iterating over, it's fine to add a break like so:

...
user_data["watchlist"].remove(movie)
break


rating_list=[]
for movie in user_data["watched"]:
if movie["rating"]> 0:

Choose a reason for hiding this comment

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

Nice job not calculating zero ratings. I was initially concerned that they might have been not accounted for in the average, but you took care of it using len!

for genre in frequent_genre_counter:
if frequent_genre_counter[genre]== max_genre:
return genre

Choose a reason for hiding this comment

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

Good job with this function! One word about using max. When pulling in code from beyond what we've talked about, it's great if you could include a comment about the research that you did, and how you think it works. And it's still worth implementing similar functionality a few times yourself, as it can help practice building our fundamental skills. For now, our goal should be getting as much coding practice as possible!

for movies in user_data["watched"]:
if movies not in same_movies:
my_movies.append(movies)
return my_movies

Choose a reason for hiding this comment

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

Great job with this function! I'd echo my concern above that submitting a PR with extraneous comments and commented out code is challenging for the reader.

for movie in friend["watched"]:
if movie not in user_data["watched"] and movie not in friend_movies:
friend_movies.append(movie)
print(f"MOVIE{movie}")

Choose a reason for hiding this comment

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

Good work on this function! I see this print statement was left in, which is usually, though not always, an unintentional debug statement. It's not a great idea to let reviewers find these, so good to get in the habit of reading your submitted PR to make sure it doesn't contain unwanted debug print statements. Totally fine once in a while, but good to be aware of.

friends_watched = get_friends_unique_watched(user_data)
rec_movie_list=[]
for friend in friends_watched:
if friend["host"] in user_data.get("subscriptions"):

Choose a reason for hiding this comment

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

Nice job using the dictionary get method!

def get_rec_from_favorites(user_data):
my_watched=get_unique_watched(user_data)
my_faves=user_data.get("favorites")
intersection=[]

Choose a reason for hiding this comment

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

intersection is a bit generic of a name, something like recs is more descriptive, which is generally a better strategy for naming variables.

# ------------- WAVE 5 --------------------
# -----------------------------------------
def get_new_rec_by_genre(user_data):

Choose a reason for hiding this comment

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

Excellent job with the Wave 5 functions!

@marciaga
Copy link

Great job on your first Unit project, Franklyn! 🟢 for Viewing Party!
Great use of variable names and helper functions as well as some syntax not yet covered in the material. I left some comments on your pull request, mostly suggestions on minor refactoring opportunities and other kudos. Great job making many small commits with clear and descriptive messages.

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.

2 participants