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

Paper- Saejin Kwak Tanguay #60

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

Conversation

saebaebae
Copy link

No description provided.

Copy link

@kaidamasaki kaidamasaki left a comment

Choose a reason for hiding this comment

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

Great job!

There were some little style things but overall your code was solid.

You definitely met all of the learning goals. 😄

Also, it looks like you named your virtual environment ven instead of venv (if you were curious why Git picked it up).

Comment on lines +5 to +15
new_movie = {}

if type(title) == str and type(genre) == str and type(rating) == float:
new_movie["title"] = title
new_movie["genre"] = genre
new_movie["rating"] = rating

return new_movie

else:
return None

Choose a reason for hiding this comment

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

You don't need to check if inputs are strings, just that they aren't None.

Also, you can clean this up a little using a dictionary literal:

Suggested change
new_movie = {}
if type(title) == str and type(genre) == str and type(rating) == float:
new_movie["title"] = title
new_movie["genre"] = genre
new_movie["rating"] = rating
return new_movie
else:
return None
if title and genre and rating:
return {
"title": title,
"genre": genre,
"rating": rating
}
else:
return None


return new_movie

else:

Choose a reason for hiding this comment

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

Style: You should avoid blank lines in front of an else or elif.

(An else or elif can't exist independent of an if so we try to "connect" them visually.)

count += 1
avg_rating = sum/count

except ZeroDivisionError:

Choose a reason for hiding this comment

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

Style: Like with else try not to put a blank line in front of except.

Comment on lines +93 to +104
for val in genre_count.values():
if val >= current_max:
current_max = val

for key, values in genre_count.items():
if values == current_max:
#winner = key
#most_watched_genre.append(winner)

#return winner

most_watched_genre.append(key)

Choose a reason for hiding this comment

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

Using more meaningful names than val or key makes code easier to read:

Suggested change
for val in genre_count.values():
if val >= current_max:
current_max = val
for key, values in genre_count.items():
if values == current_max:
#winner = key
#most_watched_genre.append(winner)
#return winner
most_watched_genre.append(key)
for count in genre_count.values():
if count >= current_max:
current_max = count
for genre, count in genre_count.items():
if count == current_max:
most_watched_genre.append(genre)

Cleaning up commented out code also helps with readability.

Comment on lines +161 to +163
if item in friends_watched_list:
continue
friends_watched_list.append(item)

Choose a reason for hiding this comment

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

It's slightly clearer to use not in instead of a continue here:

Suggested change
if item in friends_watched_list:
continue
friends_watched_list.append(item)
if item not in friends_watched_list:
friends_watched_list.append(item)

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