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

Hang - Lions #154

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

Hang - Lions #154

wants to merge 3 commits into from

Conversation

hangmtran
Copy link

This project was challenging, but I learned a lot through trial and error. Aside from list comprehensions, what other ways can I condense my code or make it more readable. Were there any methods I could have used instead? Should I include more comments about what some of lines are doing, and should I exclude some of my pseudocode type comments.

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, Hang! This project is green. 🟢
You implemented most functions according to the specification and had good code style throughout. Great use of helper functions and parsing nested data structures. Nice job finishing out the tests as well. I would recommend making more frequent commits as you progress through the milestones.
Yes, list comprehensions definitely can condense the number of lines of code, though they effectively do the same as a for loop-conditional-append structure. For some of the "get unique ..." functions, a set data type could've been used because of its helpful property of containing all unique values. It can be easily converted to a list to meet the function's return type expectation. I think your implementations were good as they are!
See my line-by-line comments for more detail. Keep up the great work!

assert updated_data["watched"][0]["title"] == MOVIE_TITLE_1
assert updated_data["watched"][0] == {
"title": MOVIE_TITLE_1,
"genre": GENRE_1,

Choose a reason for hiding this comment

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

👍🏻 Great assert statements! Love that you used all the constants provided. Makes it a very thorough test.

Comment on lines +159 to +160
assert updated_data["watched"][0]["title"] == "The Lord of the Functions: The Two Parameters"
assert updated_data["watched"][1]["title"] == "It Came from the Stack Trace"

Choose a reason for hiding this comment

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

👍🏻

def test_calculates_watched_average_rating():
# Arrange
janes_data = clean_wave_2_data()

# Act
average = get_watched_avg_rating(janes_data)
print(average)

Choose a reason for hiding this comment

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

Would advise to omit or comment out print statements in your final submission! In large projects, the console is great for debugging but when it's used in production or by other developers who are unfamiliar with your code, an excess of printing can clutter and confuse them as they look at the console raising questions like “What does this mean, why is it printed, where is this coming from?”

Comment on lines +126 to +130
assert updated_data["watched"][0]["title"] == MOVIE_TITLE_1
assert updated_data["watched"][0] == {
"title": MOVIE_TITLE_1,
"genre": GENRE_1,
"rating": RATING_1 }

Choose a reason for hiding this comment

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

👍🏻 Great assert statements! Love that you used all the constants provided by testing the whole dictionary value. Makes it a very thorough test.

# *************************************************************************************************
# ****** Add assertions here to test that the correct movies are in friends_unique_movies **********
# **************************************************************************************************

@pytest.mark.skip()
assert INTRIGUE_3 in friends_unique_movies

Choose a reason for hiding this comment

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

👍🏻

Comment on lines +106 to +116
friends_watched_titles = []
for friend in user_data["friends"]:
for movie in friend["watched"]:
if movie["title"] not in friends_watched_titles:
friends_watched_titles.append(movie["title"])



# -----------------------------------------
# ------------- WAVE 4 --------------------
# -----------------------------------------
unique_movies = []
for user_watched_movie in user_data["watched"]:
if user_watched_movie["title"] not in friends_watched_titles:

Choose a reason for hiding this comment

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

Great logic and parsing through this nested data structure.

if movie not in user_list:
friend_unique_movies.append(movie)
#removing duplicates
friend_unique_no_duplicate = [i for n, i in enumerate(friend_unique_movies)

Choose a reason for hiding this comment

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

Interesting application of enumerate! Would recommend variable names that are more descriptive so one would more easily know what is represented by i and n.


for friend in user_data["friends"]:
for movie in friend["watched"]:
if movie["host"] in user_data["subscriptions"] and movie not in user_data["watched"]:

Choose a reason for hiding this comment

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

Nice compound conditional!

# we have to make a list of recommended movies from the friend's unique watched list IF it is in the most frequently watched genre

recommended_movies = []
most_watched_genre = get_most_watched_genre(user_data)

Choose a reason for hiding this comment

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

Yes! This helper function is very handy here.

# make a list of recommendations from user's favorite

recommendations =[]
user_watched_movies = get_unique_watched(user_data)

Choose a reason for hiding this comment

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

Again, great use of 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