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

Final-viewing-party C15 -Paper- Karla T. #69

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ktiktok96
Copy link

No description provided.

Copy link
Collaborator

@CheezItMan CheezItMan left a comment

Choose a reason for hiding this comment

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

Nice work Karla, you hit the learning goals here. For improvement I did notice 3 things to work on.

  1. Try using meaningful variable names (i.e. not x, y and z). It makes the code easier to read and debug.
  2. You don't need to loop through a dictionary if you already know the key you want. See my inline comments for an example.
  3. When you functions get really long (like in wave 5). You can break them up with helper functions. That makes things easier to debug, read and test.

Otherwise this was an outstanding 1st project, you should be proud!

@@ -1,143 +0,0 @@
.DS_Store
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the future don't delete the .gitignore file please.

@@ -0,0 +1,251 @@
#WAVE ONE

def create_movie(title, genre, rating):
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

else:
return None

def add_to_watched(user_data, movie):
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

user_data["watched"].append(movie)
return user_data

def add_to_watchlist(user_data, movie):
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

user_data["watchlist"].append(movie)
return user_data

def watch_movie(user_data, title):
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

return final_list


def get_friends_unique_watched(user_data):
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍
With similar suggestions to the prior functions.


#WAVE FOUR

def get_available_recs(user_data):
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 With similar comments to the above, variable names and extra looping.

for key,value in z.items():
if z["genre"] in most_genre:
genre_rec.append(z)
#Removing duplicate genres
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you mean duplicate movie recs

return genre_rec_unq


def get_rec_from_favorites(user_data):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question could you use a prior method to get the friends watched movies, or could you make a helper method to break that part up?

Comment on lines +232 to +235
for x in user_data["friends"]:
for y in x["watched"]:
for a,b in y.items():
friend_watched.append(b)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This step would make an excellent helper method.

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