Skip to content

Tigers Neema H#155

Open
n2020h wants to merge 10 commits intoAda-C18:masterfrom
n2020h:master
Open

Tigers Neema H#155
n2020h wants to merge 10 commits intoAda-C18:masterfrom
n2020h:master

Conversation

@n2020h
Copy link
Copy Markdown

@n2020h n2020h commented Sep 23, 2022

I am interested about how I could or should have used helper functions in this project.

Copy link
Copy Markdown

@apradoada apradoada left a comment

Choose a reason for hiding this comment

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

GREEN! Overall, Neema, I love how detailed your code is and how easy it is to see your thought process. With that in mind, there are definitely some places where you can cut down on your code and just streamline it a bit! Also, feel free to work on using your helper functions to your advantage!

Comment thread tests/test_wave_01.py
# Assert
assert len(updated_data["watchlist"]) == 0
assert len(updated_data["watched"]) == 1
assert updated_data['watched']==[{'title': 'It Came from the Stack Trace', 'genre': 'Horror', 'rating': 3.5}]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This assert is definitely on the right track, but in an instance like this, it is best practice to use constants in your assert as opposed to hard coded values. This would mean replacing 'It Came from the Stack Trace' with MOVIE_TITLE_1, 'Horror' with GENRE_1 and 3.5 with RATING_1.

Comment thread tests/test_wave_01.py
# Assert
assert len(updated_data["watchlist"]) == 1
assert len(updated_data["watched"]) == 2
assert updated_data["watched"] ==[{'title': 'The Lord of the Functions: The Two Parameters', 'genre': 'Fantasy', 'rating': 4.0}, {'title': 'It Came from the Stack Trace', 'genre': 'Horror', 'rating': 3.5}]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same kind of deal as the earlier test. An easier way to approach this would be to simply make sure the movie we're moving from "watchlist" to "watched" is actually in the list. That assert would be as follows:

 assert movie_to_watch in updated_data["watched"]

Comment thread tests/test_wave_01.py
assert movie_to_watch not in updated_data["watchlist"]
assert movie_to_watch not in updated_data["watched"]
assert updated_data['watchlist']==[{'title': 'The Lord of the Functions: The Fellowship of the Function', 'genre': 'Fantasy', 'rating': 4.8}]
assert updated_data['watched']==[{'title': 'The Lord of the Functions: The Two Parameters', 'genre': 'Fantasy', 'rating': 4.0}] No newline at end of file
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Once again, rather than rewriting the entire dictionary, we can simply use the constants that we have had before!

Comment thread tests/test_wave_03.py
assert len(friends_unique_movies) == 3
assert INTRIGUE_3 in friends_unique_movies
assert HORROR_1 in friends_unique_movies
assert FANTASY_4 in friends_unique_movies
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

These are all great!

Comment thread tests/test_wave_05.py
# Assert
assert len(recommendations) == 0

#raise Exception("Test needs to be completed.")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

These look great!

Comment thread viewing_party/party.py
#create list of all hosts that are available (all_available_hosts)
all_available_hosts=[]
for host in user_data["subscriptions"]:
all_available_hosts.append(host)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Once again, you're creating a list (all_available_hosts) that already exists (user_data["subscriptions"]). That would make sense if we were modifying the list in some way, but since we are just walking through the list later, the extra list becomes redundant. You could also simply do the following and you would achieve the same result without the for loop:

 all_available_hosts = user_data["subscriptions"]

Comment thread viewing_party/party.py
if movie_to_recommend in friends_unique_watched:
recommended_movie_list.append(movie_to_recommend)

return recommended_movie_list
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The rest of this looks ok!

Comment thread viewing_party/party.py
fav_genre=get_most_watched_genre(user_data)

#call get_friends_unique_watched(user_data) to check conditions for recommendation_list
friends_unique_watched=get_friends_unique_watched(user_data)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is a great use of the helper functions as well!

Comment thread viewing_party/party.py
if movie['genre']==fav_genre:
rec_movie_list_by_genre.append(movie)
return rec_movie_list_by_genre

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This looks perfect!

Comment thread viewing_party/party.py
movies_friends_have_watched=[]
for friend in user_data['friends']:
for movie in friend['watched']:
movies_friends_have_watched.append(movie)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Use a helper function here!

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