Skip to content

Comments

Spruce - Gabe K#80

Open
GabeKelemen wants to merge 7 commits intoAda-C16:masterfrom
GabeKelemen:master
Open

Spruce - Gabe K#80
GabeKelemen wants to merge 7 commits intoAda-C16:masterfrom
GabeKelemen:master

Conversation

@GabeKelemen
Copy link

No description provided.

Copy link

@anselrognlie anselrognlie left a comment

Choose a reason for hiding this comment

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

Nice job!

All waves are complete and all tests are passing! Your code is well organized, and you are generally using good, descriptive variable names. You are making good choices about which loops to use.

One thing to try would be to make use of some auxiliary data structure to speed up some of your in checks in loops. Especially since a lot of the lookups involve checking for titles, which are immutable, a sett would be a great way to achieve O(1) lookups, especially when nested in an outer loop already.

Another opportunity would have been to use a dict as a frequency map (Learn has an example in Nested Data and Nested Loops, Nested Loops) as opposed to using list count in a nested loop.

For the size of our sample data, it wasn't strictly necessary, but getting some practice now can help for in the future when we might need to take a similar approach for much larger data!

But overall, great job!

@@ -1,5 +1,7 @@
# Viewing Party

?just testing git

Choose a reason for hiding this comment

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

LOL

@@ -0,0 +1,184 @@
# Wave 1

def create_movie(movie_title, genre, rating):

Choose a reason for hiding this comment

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

👍

Comment on lines +19 to +20
if not "watched" in user_data:
user_data["watched"] = [movie]

Choose a reason for hiding this comment

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

Ooh. Fancy!

FWIW, the use of keys to access data related to the user is really a stand-in for objects, that we'll be getting to shortly! In that scenario, we can generally assume that required "keys" are present without needing to check for them, and since Python expects us to be responsible coders, making an assumption like that, and letting things crash if the user supplied a malformed data structure wouldn't be that unexpected. 😆

return user_data


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.

👍

Comment on lines +42 to +43
movie_found = False
index_to_be_removed = -1 # set to -1 bcs invalid, can't set it to other bcs those would be valid

Choose a reason for hiding this comment

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

Strictly speaking, -1 isn't invalid (since python allows negative indexing), but you can decide that it's invalid for this situation! Since you're using a sentinel value for the index, you could even do without movie_found, and when you want to decide whether you found something later on, simply check for index_to_be_removed >= 0


def get_available_recs(user_data):

host_list = []

Choose a reason for hiding this comment

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

VSCode should be warning you about unused variables (they should appear sort of grayed out in the editor).

host_list = []
recommendation_list = []
subscription_list = user_data["subscriptions"]
user_not_watched_list = get_friends_unique_watched(user_data)

Choose a reason for hiding this comment

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

Great function reuse!

user_not_watched_list = get_friends_unique_watched(user_data)

for movie_dict in user_not_watched_list:
if movie_dict["host"] in subscription_list:

Choose a reason for hiding this comment

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

We expect the subscription list to be short, so the list in is probably ok, but we could consider using a set.

Comment on lines +163 to +164
if len(friends_watched_list) == 0:
return new_rec_list

Choose a reason for hiding this comment

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

This is a nice guard clause (does a quick check and bails out of the function so we don't need to worry about it during the rest of the function). But Notice that even without this check, if friends_watched_list was empty, the retuned value would still be an empty list.

Also, a more pythonic way to check for an empty list is

if not friends_watched_list:

friends_not_watched_list = get_unique_watched(user_data)

for movie_dict in favorites_list:
if movie_dict in friends_not_watched_list:

Choose a reason for hiding this comment

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

We could consider making a helper data structure consisting of the set of all movie titles unique to the user. Then here we could perform an equivalent check by seeing whether the title of the movie_dict is in that set. O(1) lookup!

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