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

Tigers Lilian Mora #164

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

Conversation

lilianashley
Copy link

Hi Claire I will try to fix this by Saturday 9/24/2022! thank you.

@lilianashley lilianashley changed the title project submission 9/23/22 Tigers Lilian Mora Sep 23, 2022
Copy link

@spitsfire spitsfire left a comment

Choose a reason for hiding this comment

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

Nicely done, Lily! Great job passing all tests!

I've added some feedback throughout your project to point out some places we can refactor in order to follow common conventions and standard practices when it comes to Python.

Things to consider as you work on future projects:

  • Get used to putting your edge case conditionals (aka guard clauses) at the tippy top of your functions.
  • Stay consistent with your syntax. When assigning in Python, it is most common to put a space before and after the = sign.

Comment on lines +3 to +8

from viewing_party.party import *
from dataclasses import dataclass
#from symbol import subscript
from xml.dom import UserDataHandler

Choose a reason for hiding this comment

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

I don't think we need any of these imported packages. Sometimes VSCode tried to help us by auto-importing these lines when they think we are trying to type. Sometimes VSCode gets it very wrong.

Let's remove all of these

Suggested change
from viewing_party.party import *
from dataclasses import dataclass
#from symbol import subscript
from xml.dom import UserDataHandler

Comment on lines +11 to +19
movie_dictionary = {}
if title and genre and rating:
movie_dictionary["title"] = title
movie_dictionary["genre"] = genre
movie_dictionary["rating"] = rating
return movie_dictionary

else:
return None

Choose a reason for hiding this comment

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

nice job! 👍 I'd suggest flipping the sense of the condition around to handle the special case (that something is falsy) quickly and get out, rather than having that dangling else at the end. This arrangement is referred to as a guard clause or a guard condition.

Notice that it lets us move the main focus of the function (creating the new dict) out of an indented block, letting it stand out more clearly.

def create_movie(movie_title, genre, rating):
 if not movie_title or not genre or not rating:
     return None

 movie_dict = {'title':movie_title, 'genre':genre, 'rating':rating}
 return movie_dict

return None

def add_to_watched(user_data, movie):

Choose a reason for hiding this comment

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

make sure there are no blank lines between the function signature and the first indented line:

Suggested change

Comment on lines +25 to +27
#"watched": [{"title": "{movie} "genre": "Horror", "rating": 3.5},
# {"title": "Title A", "genre": "Horror", "rating": 3.5},
# {"title": "Title A", "genre": "Horror", "rating": 3.5}]

Choose a reason for hiding this comment

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

let's get rid of any commented out code or test data we don't need when we turn in our final submission

Suggested change
#"watched": [{"title": "{movie} "genre": "Horror", "rating": 3.5},
# {"title": "Title A", "genre": "Horror", "rating": 3.5},
# {"title": "Title A", "genre": "Horror", "rating": 3.5}]

# {"title": "Title A", "genre": "Horror", "rating": 3.5},
# {"title": "Title A", "genre": "Horror", "rating": 3.5}]

def add_to_watchlist(user_data, movie):

Choose a reason for hiding this comment

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

👍

def get_new_rec_by_genre(user_data):
recommended= []
friends_movies = get_friends_unique_watched(user_data)
user_most_watched= 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.

Suggested change
user_most_watched= get_most_watched_genre(user_data)
user_most_watched = get_most_watched_genre(user_data)

return recommended

def get_rec_from_favorites(user_data):
recommended= []

Choose a reason for hiding this comment

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

Suggested change
recommended= []
recommended = []

recommended.append(movie)
return recommended

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.

👍

# -----------------------------------------
# ------------- WAVE 5 --------------------
# -----------------------------------------

def get_new_rec_by_genre(user_data):

Choose a reason for hiding this comment

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

👍

# -----------------------------------------
# ------------- 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.

👍

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