-
Notifications
You must be signed in to change notification settings - Fork 135
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
Kunzite: G Quinn #109
base: main
Are you sure you want to change the base?
Kunzite: G Quinn #109
Conversation
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.
Overall great job! The code was well-structured and follows good variable naming conventions. I'd suggest a little caution with syntax to make sure Python conventions are followed and consider refactoring opportunities whenever they might come up.
@@ -1,11 +1,127 @@ | |||
import random |
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.
Good idea to add a line break between the imports and functions in a file like this.
def draw_letters(): | ||
pass | ||
letters =[] | ||
LETTER_POOL = { |
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 dictionary could be moved outside this function into the main scope of the module so that draw_letters
doesn't need to recreate this dictionary every time the function is run.
letter = random.choice(list(LETTER_POOL)) | ||
if LETTER_POOL[letter] > 0: | ||
LETTER_POOL[letter] -= 1 | ||
else: |
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.
One suggestion - rather than have an else
block that skips the append operation on line 39, why not just append within the if
block?
|
||
def uses_available_letters(word, letter_bank): | ||
pass | ||
new_word=word.upper() |
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.
Careful with the spacing, should have space on either side of the equals sign in Python variable assignments:
new_word = word.upper()
pass | ||
new_word=word.upper() | ||
letter_bank_copy =list(letter_bank) | ||
for letter in new_word: |
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 perfectly well, but could be simplified a bit:
for letter in word.upper():
if letter in letter_bank_copy:
letter_bank_copy.remove(letter)
else:
return False
return True
maximum = scores_dict[scores] | ||
max_key = scores | ||
#Line 111 & 112 doesn't work if there are 3 or more words that are the same length |
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, this is close and passes the tests. Great job identifying where code can be improved.
|
||
# how can I do this using max()? | ||
# I originally had this but didn't how how to break ties according to len() using max(). |
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.
You'd probably need some additional conditional logic to make use of max in that way. There are a lot of different ways to accomplish the same solution, you'll see that time and again in programming!
No description provided.