-
Notifications
You must be signed in to change notification settings - Fork 69
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
Sea Turtles - Adagrams - San and Olive #50
base: master
Are you sure you want to change the base?
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.
Great job San and Olive! Your code is passing all tests and was readable. I left some comments on your PR for things you did well and some suggestions for refactoring. Please let me know if you have any questions.
from random import shuffle | ||
from collections import Counter | ||
|
||
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.
good job defining these variables as "constants" outside the function. Moving large chunks of data out from a function helps declutter the function and makes it more readable.
pass | ||
letters = [] | ||
# Create a new list of all the elements of letter pool by their count | ||
letter_bag = list(Counter(LETTER_POOL).elements()) |
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.
Calling list() on a dictionary takes all the keys and puts them in a list so we get a list of 26 letters here.
In order to select a random letter from a pool that reflects what you have declared in LETTER_POOL
, you could create a list pool_of_letters and use the keys and values from LETTER_POOL
to populate a list with 9 As, 2 Bs, 2 C,s, etc.
A suggestion could be:
pool_of_letters = []
for letter, number in available_letters.items():
pool_of_letters += letter * number
# You could then do user `random.choice(pool_of_letters)`
# Create a new list of all the elements of letter pool by their count | ||
letter_bag = list(Counter(LETTER_POOL).elements()) | ||
# Randomly reorganize the letters in letter_bag | ||
shuffle(letter_bag) |
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.
shuffle is a great method to use here. If you didn't use shuffle here how would you get random words? Make sure if you were in a situation like interviewing you know how to explain how this method works or be able to implement an approach that doesn't use a method.
while len(letters) < 10: | ||
letters.append(letter_bag.pop()) | ||
return letters | ||
|
||
|
||
def uses_available_letters(word, letter_bank): |
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.
Great job making a copy here. For example, you are removing letters from the original letter_bank
, but the function returns False, meaning the word can't be made. Well, if you remove all the letters from the list, now the player's hand is empty! and they can't play the round anymore.
else: | ||
letter_bank_copy.remove(letter) | ||
return True | ||
|
||
|
||
def score_word(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.
Great approach. This is about as efficient as we can get for this function!
# Create a dictionary of the words from word_list and their scores | ||
for word in word_list: | ||
word_scores[word] = score_word(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.
good use of score_word
here
if len(word) >= 7: | ||
total += 8 | ||
return total | ||
|
||
|
||
def get_highest_word_score(word_list): |
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.
Another approach would be creating a list or dictionary that contained only the highest scoring words in a single loop and then tie-breaking if that data structure contains more than one item. I would consider creating a helper function to manage tie breaking to have smaller, more focused functions.
No description provided.