Skip to content

Snow Leopards - Diana S. #167

Open
sorindevops wants to merge 13 commits intoAda-C18:masterfrom
sorindevops:master
Open

Snow Leopards - Diana S. #167
sorindevops wants to merge 13 commits intoAda-C18:masterfrom
sorindevops:master

Conversation

@sorindevops
Copy link
Copy Markdown

No description provided.

Comment thread play_tester.py
pp.pprint(HORROR_1)
pp.pprint(FANTASY_1)
pp.pprint(FANTASY_2)
print(create_movie(MOVIE_TITLE_1,GENRE_1,RATING_1))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just a little FYI, in the future, if you make changes to a sandbox file like this, you don't need to add it to the staging area/commit it and push up the changes.

It's no problem that you did so here since this isn't a PR that will get merged into the project, but if your changes were to be merged into a project then someone reviewing your code would probably ask you to remove these changes from the PR so they don't get merged into the main code base.

Comment thread tests/test_wave_01.py

raise Exception("Test needs to be completed.")
# raise Exception("Test needs to be completed.")
assert MOVIE_TITLE_1 == updated_data["watched"][0]["title"]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You could also assert against the genre and rating key/values in the same way, for more thoroughness!

Comment thread tests/test_wave_01.py

raise Exception("Test needs to be completed.")
# raise Exception("Test needs to be completed.")
assert updated_data["watched"] == [FANTASY_2,HORROR_1]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This definitely works! You could also use in to make the assertions and capture more conditions like:

    assert updated_data["watched"][1] == movie_to_watch
    assert movie_to_watch in updated_data["watched"]
    assert FANTASY_2 in updated_data["watched"]
    assert FANTASY_1 in updated_data["watchlist"]
    assert movie_to_watch not in updated_data["watchlist"]

Comment thread tests/test_wave_03.py

raise Exception("Test needs to be completed.")
for movie in friends_unique_movies:
assert movie in [HORROR_1, FANTASY_4, INTRIGUE_3]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍

Comment thread tests/test_wave_05.py
}

raise Exception("Test needs to be completed.")
# raise Exception("Test needs to be completed.")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This test still has a skip on it and when I ran it, the test failed. I'll leave more detailed comments in Wave 5 in party.py

Comment thread viewing_party/party.py
@@ -1,23 +1,166 @@
# ------------- WAVE 1 --------------------
from pprint import pprint
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

An extraneous import? Probably auto-imported from VSCode. These unused imports should not be committed. I recommend turning off that setting in VS Code.

Comment thread viewing_party/party.py
def _get_movies_watched_by_friends(user_data):
movies_watched_by_friends = []

for friend in user_data["friends"]: #[{}]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Not sure what the purpose of the comment at the end of this line is, but it's safe to delete it.

Comment thread viewing_party/party.py

return movies_watched_by_friends

def _get_movies_not_watched_user(user_data):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This function appears to not be used so there's no need to commit it

Comment thread viewing_party/party.py

for friend in user_data["friends"]: #[{}]
for friend_movie in friend["watched"]: #{}
if friend_movie in movies_watched_by_friends:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This conditional could be rewritten to avoid using continue in the if block, I believe as follows:

            if friend_movie not in movies_watched_by_friends:
                movies_watched_by_friends.append(friend_movie)     

Comment thread viewing_party/party.py
watchlist_title = movie["title"]
if title == watchlist_title:
user_data["watchlist"].remove(movie)
user_data["watched"].append(movie)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

To avoid any potential issues modifying a list that's being iterated on, it's a good idea to break after making that modification.

Comment thread viewing_party/party.py
for movie in user_data["watched"]:
rating += movie["rating"]

if rating == 0:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Great job with this method! Couple of small callouts here.

You could convey intent a little more explicitly by pulling the logic around return 0 into it's own guard clause with (on line 57 so it's the first line of code that executes in this method):

if not user_data["watched"]:
      return 0

then proceed to build up your rating with the for-loop.

This way, when another developer comes along, I can immediately recognize "Oh if this value doesn't exist, then we'll return 0. The way your code is currently structured, I can tell at some point this method will return 0, but it's not as immediately clear.

Also a note about guard clauses - The Guard Clause pattern helps in these cases by “inverting” that way of thinking and rapidly returning false conditions and keeping the important code blocks (usually the longest) in the lowest indentation level possible

Comment thread viewing_party/party.py
genre_list.append(genre) #each genre will be added to the genre_list # the congregate of all genres


favorite_genre = max(set(genre_list), key= genre_list.count)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice use of max here. I find it useful to also practice writing your own implementation of finding a max value from a dictionary. You may find in an interview if you use built in functions like max that an interviewer may ask you how the function works under the hood. So knowing how to write it on your own can help you better understand the function.

Comment thread viewing_party/party.py
# -----------------------------------------
# ------------- WAVE 3 --------------------
# -----------------------------------------
def get_unique_watched(user_data):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good use of helper function!

Comment thread viewing_party/party.py
for friend_movie in movies_watched_by_friends: #user_data = "friends": [{}]
if friend_movie not in user_watched_movies:
recommended_movies.append(friend_movie)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good job with this one!

Comment thread viewing_party/party.py

for movie in recommended_movies:
if movie["host"] in user_subscriptions:
pprint(recommended_movies_subscription.append(movie))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this expression should be taken out of the pprint call

Comment thread viewing_party/party.py
if friend_movie["genre"] == popular_genre:
if friend_movie not in user_data["watched"]:
list_rec_movies_genre.append(friend_movie)
print(list_rec_movies_genre)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It's usually not a good idea to leave print statements in committed code because they're often for debug purposes. It's good to get in the habit of re-reading your PRs to make sure they don't contain things like this.

Comment thread viewing_party/party.py


def get_rec_from_favorites(user_data):
list_rec_favorites= []
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In order to fix that failing test, using the code in this function, you'll have to guard against the possibility that there's no "favorites" key in the user_data dictionary. There's a dictionary method that returns None if the key doesn't exist, called get. You can use it like so:

    if not user_data.get("favorites"):
        return []

If you use that as the guard clause, then allow that test to run, it should pass.

@marciaga
Copy link
Copy Markdown

Great job on your first Unit project, Diana!
Good use of variable names and helper functions as well as some syntax not yet covered in the material. I left some comments on your pull request, mostly suggestions on refactoring opportunities and other things to consider. Great job making many small commits with clear and descriptive messages.

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