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

AC2 Ocelots Class - Catherine Bandarchuk #1

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

Conversation

CatherineBandarchuk
Copy link

No description provided.

Copy link

@kelsey-steven-ada kelsey-steven-ada left a comment

Choose a reason for hiding this comment

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

Congrats on completing your first project at Ada! I’ve added some suggestions & questions, let me know if there's anything I can clarify ^_^

Comment on lines +114 to +116
assert updated_data["watched"][0]["title"] == MOVIE_TITLE_1
assert updated_data["watched"][0]["genre"] == GENRE_1
assert updated_data["watched"][0]["rating"] == RATING_1

Choose a reason for hiding this comment

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

Nice checks for all the movie's data!

Comment on lines 135 to 137
assert updated_data["watched"][0]["title"] == movie_to_watch["title"]
assert updated_data["watched"][0]["genre"] == movie_to_watch["genre"]
assert updated_data["watched"][0]["rating"] == movie_to_watch["rating"]

Choose a reason for hiding this comment

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

The README isn't explicit about what order we should add movies to the list watched. If we want our tests to be independent from a specific implementation that adds movies to a particular location in the watched list, we could use the in keyword to check if the variable movie_to_watch is in the watched list instead of looking at a specific index:

assert len(updated_data["watchlist"]) == 1
assert len(updated_data["watched"]) == 2
assert movie_to_watch in updated_data["watched"]

Comment on lines 54 to 60
list_of_title = list()
for i in range(len(friends_unique_movies)):
list_of_title.append(friends_unique_movies[i]["title"])

# Assert
assert len(friends_unique_movies) == 3

raise Exception("Test needs to be completed.")
# *************************************************************************************************
# ****** Add assertions here to test that the correct movies are in friends_unique_movies **********
# **************************************************************************************************

@pytest.mark.skip()
assert len(set(list_of_title)) == len(list_of_title)

Choose a reason for hiding this comment

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

Here, we compare the length of the set of titles vs the length of the list of titles. This confirms that the number of titles is correct, but it doesn't guarantee that their values are what we expect. I think we could also say this check duplicates the info that the previous assert assert len(friends_unique_movies) == 3 gives us.

If we explore the clean_wave_3_data test data in the test_constants file, we could reason about which titles should be in our result and directly assert on those. This would also reduce the time the test takes to run since we could avoid creating a list of the titles and then a set from that list.

assert len(friends_unique_movies) == 3
assert INTRIGUE_3 in friends_unique_movies
assert HORROR_1 in friends_unique_movies
assert FANTASY_4 in friends_unique_movies


def create_movie(title, genre, rating):
pass
new_movie = dict()

Choose a reason for hiding this comment

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

I'd suggest moving the new_movie declaration under the invalid data check so that we don't allocate space for new_movie unless we know it will be used.

Comment on lines 6 to 12
if title == None or genre == None or rating == None:
return None
else:
new_movie["title"] = title
new_movie["genre"] = genre
new_movie["rating"] = rating
return new_movie

Choose a reason for hiding this comment

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

If we do not explicitly need an else clause, I suggest leaving it off, as it is more more statement that needs to be evaluated, and often causes the main flow of our function that we want to be very clear to readers to be indented. We could remove the else here without changing the outcome of our function:

if title == None or genre == None or rating == None:
    return None

new_movie = dict()
new_movie["title"] = title
new_movie["genre"] = genre
new_movie["rating"] = rating
return new_movie

# another option could be:
if title == None or genre == None or rating == None:
    return None

return {
    "title": title,
    "genre": genre,
    "rating": rating 
}

Comment on lines 103 to 104
if not user_data["watched"] or not unique_friends_movie or not user_data["subscriptions"]:
return list_of_recommended_movies

Choose a reason for hiding this comment

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

Nice choice to exit early if the relevant data is missing.

Comment on lines 122 to 124
for movie in user_data["watched"]:
list_of_genre.append(movie["genre"])
most_ofen_genre = statistics.mode(list_of_genre)

Choose a reason for hiding this comment

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

It looks like lines 122-124 duplicates what we do in get_most_watched_genre. How could we re-write this to use our previous function get_most_watched_genre instead?

list_of_genre.append(movie["genre"])
most_ofen_genre = statistics.mode(list_of_genre)
list_of_movies = get_friends_unique_watched(user_data)
if not list_of_movies or not list_of_genre:

Choose a reason for hiding this comment

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

We could forgo this if-check without impacting the outcome of the function. If list_of_movies is empty, then we'd never enter the loop on line 129, we don't need extra checks to prevent it.

list_of_movies = get_friends_unique_watched(user_data)

for i in range(len(list_of_movies)):
    if list_of_movies[i]["genre"] == most_ofen_genre:
        recommended_movies.append(list_of_movies[i])

return recommended_movies

unique_movies = get_unique_watched(user_data) #user's watched movies, different from friend's movies
for i in range(len(user_data["favorites"])):
favorite_movies.append(user_data["favorites"][i])
for movie in unique_movies:

Choose a reason for hiding this comment

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

Great use for a for-in loop!

Comment on lines 141 to 142
for i in range(len(user_data["favorites"])):
favorite_movies.append(user_data["favorites"][i])

Choose a reason for hiding this comment

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

It looks like favorite_movies is an unchanged copy of user_data["favorites"]. If that's the case, then we could factor out favorite_movies and use user_data["favorites"] directly on line 144 below:

for movie in unique_movies:
    if movie in user_data["favorites"]:
        recommended_movies.append(movie) 

Copy link

@kelsey-steven-ada kelsey-steven-ada left a comment

Choose a reason for hiding this comment

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

I like how you incorporated the feedback! There was only one note around the antipattern of continuing to loop after making changes to a data structure that I think is really important to make sure we understand what is happening and why we want to avoid it.

Otherwise, around code-styles, in the future I'd consider adding some blank lines inside of functions to visually separate chunks of code. For example, I like to add a blank line after my validation check(s) to give readers a visual separation between the validation code and the main flow of the function. I might also use a blank line to separate chunks of code that perform subtasks of the overall goal.

Comment on lines +24 to +29
for i in range(len(user_data["watchlist"])):
movie = user_data["watchlist"][i]
if movie["title"] == title:
user_data["watched"].append(movie)
user_data["watchlist"].remove(movie)
return user_data

Choose a reason for hiding this comment

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

We're still running into an antipattern of continuing to loop after removing an element of the list we're looping over. I would recommend keeping the for-in loop syntax and moving the return inside the if-statement something like:

for movie in user_data["watchlist"]:
    if movie["title"] == title:
        user_data["watched"].append(movie)
        user_data["watchlist"].remove(movie)
        return user_data

@CatherineBandarchuk
Copy link
Author

CatherineBandarchuk commented Nov 8, 2022 via email

@kelsey-steven-ada
Copy link

That is a great question. If we declare our loop with for i in range(len(<list_name>)): the length gets evaluated when the loop is created, but it does not keep getting updated as the loop progresses. This is part of why we see an IndexError after the first couple iterations if we try to run code like:

list_a = ["a", "b", "c", "d"]
for i in range(len(list_a)):
    print(list_a[i])
    del list_a[i]

The code above will output "a", then "c", then crash with IndexError: list index out of range

  • Our range is initialized to a sequence of values from 0 to 3
  • In the first iteration, i is 0 so we print then delete "a"
  • In the second iteration, i is 1. We deleted "a", so now "b" is at index 0 and "c" is at index 1. This means we will skip "b" and print then delete "c" at index 1
  • In our third iteration, i is 2 and "a" and "c" have been deleted leaving "b" at index 0 and "d" at index 1. When we hit the line print(list_a[i]) we will now crash since i is 2, but we have no element at index 2.

@CatherineBandarchuk
Copy link
Author

CatherineBandarchuk commented Nov 8, 2022 via email

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.

3 participants