-
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
Sharks - Morgan B. and Philomena K. #55
base: master
Are you sure you want to change the base?
Conversation
…e and optimized for Big O
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 work on this project Morgan and Philomena!
You both passed the tests and met the learning goals. I left a couple of small comments about naming variables/functions and optimizations.
You both have earned a 🟢 on Adagrams!
import random | ||
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.
I like that this defined as a "constant" outside the function. Moving a large chunk of data out from a function helps declutter the function.
And I see what you did there with the letter frequency and letter score in the list. You might even consider renaming LETTER_POOL to something like LETTER_FREQ_WITH_SCORES to indicate that this dict holds 2 different kinds of information.
|
||
Returns: | ||
returned_letters (list): list of 10 randomly chosen letters | ||
""" |
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, succinct docstring here.
returned_letters = [] | ||
for letter, number in LETTER_POOL.items(): | ||
letter_list += letter * number[0] | ||
for i 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.
You could also write this loop like this while len(letters) < 10:
""" | ||
Checks if the provided word utilizes letters only found in letter bank | ||
|
||
Parameters: | ||
word (str): the played word | ||
letter_bank (list): bank of randomly assigned letters | ||
|
||
Returns: | ||
boolean value based on word validity | ||
""" |
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.
👍
Returns: | ||
boolean value based on word validity | ||
""" | ||
letter_dict = Counter(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 building a frequency map from the letters to efficiently check whether we can make the supplied word!
|
||
for letter in word.upper(): | ||
try: | ||
if letter in letter_dict.keys() and letter_dict[letter] > 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.
Take care to use key in dict syntax
. If you dokey in dict.keys()
you'd reproduce O(n) time complexity since under the hood Python creates a list of keys when you call .keys() and the in operator for lists is linear.
if letter in letter_dict and letter_dict[letter] > 0:
except KeyError: | ||
return False | ||
|
||
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.
👍
|
||
Analyzes high scores to check for a tie. | ||
Follows game rules for tie breaker: | ||
|
||
Ties are judged based off of letter count: | ||
|
||
If Their is a 12 letter word present, returns the | ||
first 12 letter word that appears in the list. | ||
|
||
For cases without a 12 letter word, returns the | ||
shortest word in the list. | ||
|
||
If the shortest words are the same length, return | ||
the one that appears first in the original 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.
There aren't hard and fast rules about writing docstrings. Some devs, myself included, prefer shorter docstrings where possible. I think calling out the input and outputs suffices since I can gather how ties are handled by reading the code. I think that you could leave this first chunk out and I'd still understand what this function does
word_length = len(word) | ||
if word_length == 10: | ||
return word, top_score | ||
elif not lowest: | ||
lowest = (word, top_score, word_length) | ||
elif word_length < lowest[2]: | ||
lowest = (word, top_score, word_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.
You could also pull this into a helper method to simplify the logic in this function
No description provided.