Skip to content
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

Hannah Watkins Atlanta Amethyst Adagrams #120

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Hannah1Watkins
Copy link

No description provided.

Copy link

@audreyandoy audreyandoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome job Hannah ✨

Your submission covers all the learning goals and passes all the tests! You have earned a 🟢 grade!

In your PR, you'll find comments on ways to refactor and DRY up your code. I see that you only made 3 commits. For future projects, please make your commits small, frequent, and with detailed commit messages! Your commit messages should describe what changes you've added to your code. For example, "added score_word logic and passed the tests". Detailed commit messages are super helpful for future bug hunting!

Otherwise, keep up the great work Hannah! 💜✨

@@ -164,7 +164,7 @@ This will let us give feedback on what you've finished so that you can be better

Take time to read through the Wave 1 implementation requirements and the tests for Wave 1. Write down your questions, and spend some time going through your understanding of the requirements and tests. Make sure you can run `$ pytest` and see the tests fail.

If, after you have taken some time to think through the problem and would like direction for how to dissect the problem, or if you need clarity on the terms/vocabulary we used in this project, you can check out [a small hint we've provided](./project_docs/hints.md).
If, after you have taken some time to think through the problem and would like direction for how to disgit add -Asect the problem, or if you need clarity on the terms/vocabulary we used in this project, you can check out [a small hint we've provided](./project_docs/hints.md).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to add this change? If not, be care of what you add to your commits!

Comment on lines +3 to +7
LETTER_SELECTION = {
'A': 9, 'B': 2, 'C': 2, 'D': 4, 'E': 12, 'F': 2, 'G': 3, 'H': 2, 'I': 9,
'J': 1, 'K': 1, 'L': 4, 'M': 2, 'N': 6, 'O': 8, 'P': 2, 'Q': 1, 'R': 6,
'S': 4, 'T': 6, 'U': 4, 'V': 2, 'W': 2, 'X': 1, 'Y': 2, 'Z': 1,
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work using the uppercase naming convention to indicate that this is a constant variable ✨

Since LETTER_POOL is accessed by one function, it would be reasonable to place them inside the functions rather than as a constant. There are tradeoffs, the structures would clutter the function some, but it keeps the data as close as possible to where it's being used, and would mean other functions couldn't access it to accidentally alter the contents.

Comment on lines 9 to +22
def draw_letters():
pass
letter_pool = dict(LETTER_SELECTION)
# list to store drawn letters
hand = []
# use while loop to draw 10
while len(hand) < 10:
letter = random.choices(list(letter_pool.keys()), list(letter_pool.values()))[0]
hand.append(letter)
letter_pool[letter] -= 1
if letter_pool[letter] == 0:
del letter_pool[letter]

# return drawn letter list
return hand

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Great work accounting for the statistical probability of pulling each letter.

Comment on lines +25 to +32
word = word.lower()
letter_bank = [letter.lower() for letter in letter_bank]
# Check if every letter in input word is available in letter_bank
for letter in word:
if letter not in letter_bank:
return False
else:
letter_bank.remove(letter)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Great work using list comprehension!!!!

We can also use lower() with word in the for loop directly:

for letter in word.lower()

Comment on lines 36 to +45
def score_word(word):
pass
points = {
'A': 1, 'B': 3, 'C': 3, 'D': 2, 'E': 1, 'F': 4, 'G': 2, 'H': 4, 'I': 1,
'J': 8, 'K': 5, 'L': 1, 'M': 3, 'N': 1, 'O': 1, 'P': 3, 'Q': 10, 'R': 1,
'S': 1, 'T': 1, 'U': 1, 'V': 4, 'W': 4, 'X': 8, 'Y': 4, 'Z': 10
}
score = sum(points.get(letter.upper(), 0) for letter in word)
if len(word) in (7, 8, 9, 10):
score += 8
return score

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥 This is a really clean solution Hannah! Love how you used comprehension syntax in sum. And utilized a tuple for checking the word length! ✨

Optionally, we could move points outside of the function for readability and to make this function look less crowded.

But other than that, excellent work!

# create empty dict using defaultdict
# assign to scores (will hold the score of each word as the key)
scores = defaultdict(list)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you decide to use defaultdict instead of dict() or a dictionary literal {}.

Comment on lines +55 to +58
# calculate score of word using score_word func. & store it in var score
score = score_word(word)
scores[score].append(word)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use dictionary comprehension to create your scores dictionary.

scores = { score_word(word): word for word in word_list }

scores[score].append(word)
# find highest score from scores dict using max func. store it in var max_score
max_score = max(scores.keys())

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The keys of a dictionary are its default iterator so we can omit keys. This means that max will automatically evaluate the keys of a dictionary by default when finding the max.

max_score = max(scores)

return max_score_words[0], max_score
# check hgihest score word length for 10, return w/ score as tuple
ten_letter_words = [w for w in max_score_words if len(w) == 10]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lovely use of list comprehension!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants