-
Notifications
You must be signed in to change notification settings - Fork 57
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
Spruce - Asha P. #43
base: master
Are you sure you want to change the base?
Spruce - Asha P. #43
Conversation
…er found and value > 0
…ll wave 2 test cases
Merge branch 'master' of https://github.com/ashapa/adagrams-py
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.
Nice job!
All waves are complete and all tests are passing! Your code is well organized, and you are generally using good, descriptive variable names. You used lists and dictionaries throughout, and were aware of when you needed to make copies to avoid side-effects.
All in all, this was a really concise approach throughout. I liked seeing the use of helper data structures to improve the efficiency of working with lists of tiles/words. One thing to notice is that sometimes reversing the sense of a comparison, or selecting a different loop, can help focus our code even more!
Overall, great work!
import copy | ||
|
||
|
||
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.
I also like having this defined as a "constant" outside the function. Theoretically, we could write our draw_letters
method to accept a pool as a parameter (defaulting to this one). But moving a large chunk of data out from a function also just helps declutter the function. 🙂
def draw_letters(): | ||
pass | ||
|
||
temp_letter_pool = copy.deepcopy(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.
Since the method used here to pick the tiles modifies the pool, we do need to make a copy, but since our pool structure is a dictionary of strings to integers (both immutable types), a shallow copy would be sufficient.
letter_not_found = True | ||
letters_drawn_list = [] | ||
|
||
letter = "" |
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.
We could move this down into the for loop (a good place might be just before the while loop).
|
||
letter = "" | ||
|
||
for draw in range(10): |
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.
A common "name" for a variable we don't really care about (but which syntax requires us to have) is _
. With a name like draw
I had to look through the code and see whether it was being used for anything.
# need this flag when exiting while loop to search for other letter | ||
letter_not_found = True | ||
|
||
while letter_not_found: | ||
letter = random.choice(letter_list_from_pool) | ||
letter_count = temp_letter_pool[letter] | ||
|
||
# this conditional checks that there's a letter available and changes the flag to letter found | ||
if letter_count > 0: | ||
letter_not_found = False | ||
|
||
letters_drawn_list.append(letter) | ||
# decrement count temp letter pool | ||
temp_letter_pool[letter] -= 1 |
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.
Personally, I tend to avoid a flag controlled loop, and prefer using an infinite loop with a break
, since then I know exactly where I leave the loop and don't have to worry about unintended code accidentally running. Maybe something like this?
while True:
letter = random.choice(letter_list_from_pool)
letter_count = temp_letter_pool[letter]
if letter_count:
letters_drawn_list.append(letter)
temp_letter_pool[letter] -= 1
break
Alternatively, rather than looping 10 times literally, we could modify the outer loop to loop until 10 things have been added to the drawn list. That would allow for an overall approach something like:
while len(letters_drawn_list) < 10:
letter = random.choice(letter_list_from_pool)
letter_count = temp_letter_pool[letter]
# if the picked letter has remaining count, use it
if letter_count:
letters_drawn_list.append(letter)
temp_letter_pool[letter] -= 1
return letters_drawn_list
if letter in letter_bank_letter_count and letter_bank_letter_count[letter] > 0: | ||
letter_bank_letter_count[letter] -= 1 | ||
else: | ||
return False |
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.
get
can help again here
if not letter_bank_letter_count.get(letter, 0):
return False
letter_bank_letter_count[letter] -= 1
pass | ||
word = word.lower() | ||
|
||
SCORE_CHART = { |
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.
Nice lookup table for the tile scores. As with LETTER_POOL
, we might move this to a "constant" outside the function so that it doesn't need to be re-created each time we call the scoring function.
Also, it's a little surprising to me that one of the letter dictionaries uses capital letters, and the other uses lowercase. Consider sticking with one or the other.
if 6 < len(word) < 11: | ||
word_score += 8 |
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.
Consider creating constants to represent these literal values, for example:
MIN_BONUS_LENGTH = 6
MAX_BONUS_LENGTH = 11
LENGTH_BONUS = 8
Something else to consider, is it even necessary to check whether the length of the word is less than 11?
|
||
# this is the comparison, set to the value of the first in list | ||
winning_word = word_list[0] |
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.
Notice that if the word_list
were empty (why? 🤷 ) this would raise an IndexError. There weren't any tests that checked for the behavior of an empty list, but maybe we could structure things to return None, 0
or some other result to indicate there was no result?
winning_word = word | ||
# if new word is shorter than the old word AND the old word isn't 10 letters, replace it | ||
elif len(word) < len(winning_word) and len(winning_word) < 10: | ||
winning_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.
Nice set of checks. There are a number of ways to group these. In fact, it's even possible to combine these into a single check! Since they both have len(winning_word) < 10
as part of the checks, can you see a way to combine them?
No description provided.