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

Daria Iakymenko (C17, Whales class) #100

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

Conversation

diakymenko
Copy link

No description provided.

Copy link

@jericahuang jericahuang left a comment

Choose a reason for hiding this comment

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

Excellent first project, Daria! You hit the learning goals and passed all the tests. 🙂🟢
You implemented every function according to the spec and had great code style throughout. Especially nice use of sets (no duplicates) and dictionaries (counting). Very nice additions to the test suite as well. See my line-by-line comments for some more detailed pointers for improvement as well as great highlights I saw. Keep up the fantastic work!



def get_most_watched_genre(user_data):
most_watched = {}

Choose a reason for hiding this comment

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

Fantastic use of a dictionary to keep track of counts!

@@ -78,6 +78,17 @@
],
}

USER_ADDITIONAL_DATA_2 = {

Choose a reason for hiding this comment

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

Nice custom test constant!

@@ -30,9 +30,9 @@ def test_create_no_title_movie():
new_movie = create_movie(movie_title, genre, rating)

# Assert
assert new_movie is None
assert new_movie == None

Choose a reason for hiding this comment

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

This check works and passes, but the preferred comparison operator is is for checking whether a variable is None. This article gives a good explanation.

Comment on lines +147 to +149
assert updated_data["watched"][1]["title"] == MOVIE_TITLE_1
assert updated_data["watched"][1]["genre"] == GENRE_1
assert updated_data["watched"][1]["rating"] == RATING_1

Choose a reason for hiding this comment

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

👍🏻

@@ -39,7 +39,19 @@ def test_most_watched_genre():
assert popular_genre == "Fantasy"
assert janes_data == clean_wave_2_data()

@pytest.mark.skip()
def test_most_watched_genre_additional(): #added an additional test to make sure the function picks up

Choose a reason for hiding this comment

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

Yes! Great additional test!


# -----------------------------------------
# ------------- WAVE 2 --------------------
# -----------------------------------------
def get_watched_avg_rating(user_data):
watched_movies_list = []
sum = 0

Choose a reason for hiding this comment

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

sum is considered a reserved word since it is a built-in function. We'd encourage to consider another variable name like summation!

for item in user_data["watched"]:
watched_movies_list.append(item["rating"])
for number in watched_movies_list:
sum += number

Choose a reason for hiding this comment

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

Nice; we also could've used the built-in sum function to sum up everything in this list by sum(watched_movies_list)



def get_friends_unique_watched(user_data):
set_watched = set()

Choose a reason for hiding this comment

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

Awesome use of a set to track unique items!

# -----------------------------------------
# ------------- WAVE 4 --------------------
# -----------------------------------------
def get_available_recs(user_data):

Choose a reason for hiding this comment

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

Great implementation, though we could've used get_friends_unique_watched as a helper function to avoid any redundancy in code logic!

return rec_by_genre_list


def get_rec_from_favorites(user_data):

Choose a reason for hiding this comment

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

Nice implementation, we could've also used get_unique_watched in both wave 5 functions as a helper function

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