Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 - Lindsey B. #156
base: master
Are you sure you want to change the base?
Tigers - Lindsey B. #156
Changes from 2 commits
89b8414
f4466a2
bb3a804
61a8333
521d978
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like an
import
statement that ended up not being used. Just a small style thing, it's usually worth removing these before submission to keep the code clean.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still here after resubmission, but not really something major, so it's fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor style thing about where you declare variables. Since we only use this
new_movie
variable in theif title and genre and rating
block, we can move its declaration there. In this case it only saves the very minor memory allocation of an empty dictionary, but that's still worth keeping in mind.In general, I look to delay my variable declarations to the last possible moment, which both avoids unnecessary allocations if the function exits early, and tends to make the variable declaration be as close as possible to where it is used, which means less scrolling if you need to check the declaration to understand what's happening later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nanoscopic style nitpick: Watch out for unneeded blank lines. Sometimes multiple blank lines are used as a style method for better separating, say functions, but that doesn't seem to be the case here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obviously not going to point out every instance of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably don't need to keep these comments around, I guess they were scaffolding comments, and once done with them, you can remove them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like the increment statement above, this increment statement will throw off your loop, causing every other item to be skipped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still applies in the updated code, which will definitely cause a bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works, but there is a small performance hit. Even though we hadn't covered time complexity at the time you wrote this, it will be helpful to understand what's going on.
When calling
genre_list.count(genre)
, it will be an O(n) operation because it will go through every item ingenre_list
to get the count. However, since we are going through everygenre
ingenre_list
in our loop, that is also going to be an O(n) operation causing the entire loop to end up O(n^2).While
count
is an incredibly useful method here, we can get this loop down to O(n) by managinggenre_dictionary
ourselves:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the updated does use
genre_dictionary
note that we still hit the O(n^2) performance issue that I pointed out last time due to callinggenre_list.count(genre)
during each iteration offor genre in genre_list
. But it's progress, so it's not too concerning.