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.

Sign up for GitHub

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

Jump to bottom

zoisite - Say Ryder #113

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

zoisite - Say Ryder #113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d15af72
Made it past wave 1
LRyder17 Mar 22, 2023
f73c9f2
made it past wave1 FOR REALZ
LRyder17 Mar 22, 2023
a1deee4
passed wave 2
LRyder17 Mar 23, 2023
513b40d
wave 3 good to go
LRyder17 Mar 23, 2023
2ded86d
wave 4 complete
LRyder17 Mar 24, 2023
b81df29
refactor of uses_available_letters_()
LRyder17 Mar 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
149 changes: 145 additions & 4 deletions adagrams/game.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,152 @@
import random


LETTER_POOL = {
Copy link

Choose a reason for hiding this comment

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

Since LETTER_POOL is only accessed by the draw_letters function, it would be reasonable to place the dictionary inside the function rather than as a constant. There are tradeoffs, the structure 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.

'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
}

def draw_letters():
pass
num_letters = 10
letters, weights = zip(*LETTER_POOL.items())
Copy link

Choose a reason for hiding this comment

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

Really neat use of zip! How might you explain what this line is doing to another person?

hand = []
valid_hand = False

while not valid_hand:
valid_letter_count = 0
hand_dict = {}
hand = random.choices(letters, weights, k = num_letters)
Copy link

Choose a reason for hiding this comment

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

Love to see folks researching into functions and their optional parameters! It's extremely unlikely given the weights, but because there is no guarantee that any particular draw would be valid, in a very rare circumstance this could cause an infinite loop. Is there another way we could approach it that guarantees the function will stop looping?


for letter in hand:
if letter in hand_dict:
hand_dict[letter] += 1
else:
hand_dict[letter] = 1

for letter in hand_dict.keys():
if hand_dict[letter] <= LETTER_POOL[letter]:
valid_letter_count += 1
Comment on lines +44 to +52
Copy link

Choose a reason for hiding this comment

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

I like the approach to use a frequency map to confirm we never go over the amounts in LETTER_POOL.


if valid_letter_count == 10:
valid_hand = True
else:
hand = random.choices(letters, weights, k = num_letters)
Comment on lines +56 to +57
Copy link

Choose a reason for hiding this comment

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

We could remove this else block without changing the outcome of the function. If we hit line 57, then valid_hand must be False, meaning we start the loop over. hand will get reassigned again on line 42 before the choices in this else can be analyzed.


return hand


def uses_available_letters(word, letter_bank):
pass
word = list(word.upper())
letter_bank = list("".join(letter_bank).upper())
Copy link

Choose a reason for hiding this comment

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

There is a bit of nesting going on here that makes the line a little harder to quickly absorb. One way we could rewrite this is with a list comprehension:

letter_bank = [letter.upper() for letter in letter_bank]

matched_letters = []

if len(word) <= len(letter_bank):
for letter in letter_bank:
Copy link

Choose a reason for hiding this comment

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

If we iterate over for letter in word: here instead, how could that simplify the function?

if word.count(letter) > letter_bank.count(letter):
return False

for letter in word:
if letter in letter_bank:
matched_letters.append(letter)
else:
return False

if len(word) == len(matched_letters):
for letter in word:
if word.count(letter) <= matched_letters.count(letter):
continue
else:
return False
return True
else:
return False
else:
return False
Comment on lines +67 to +88
Copy link

Choose a reason for hiding this comment

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

This is deeply nested which can make it unclear or harder to follow what is the intended success path of our code. We can re-write some of these as if-statements by inverting what we are checking and can turn them into validation checks that exit right there if we can:

if len(word) > len(letter_bank):
    return False

for letter in letter_bank:
    if word.count(letter) > letter_bank.count(letter):
        return False

for letter in word:
    if letter in letter_bank:
        matched_letters.append(letter)
    else:
        return False

if len(word) != len(matched_letters):
    return False

for letter in word:
    if word.count(letter) > matched_letters.count(letter):
        return False

return True


def score_word(word):
pass
score_value_dict = {
'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
}
Copy link

Choose a reason for hiding this comment

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

It looks like this closing bracket is at the same indentation level as the function name, I suggest indenting this to line up with score_value_dict.

word = list(word.upper())
Copy link

Choose a reason for hiding this comment

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

One of the cool things about strings in python is that they are iterable - we can loop through them like a list. This means we don't need to convert word.upper() to a list unless we need to do something that requires the other abilities of lists.

score = 0

if len(word) >= 7:
score += 8

for letter in word:
if letter in score_value_dict:
score += score_value_dict[letter]
Comment on lines +126 to +127
Copy link

Choose a reason for hiding this comment

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

Nice way to handle things like hyphenated words.


return score

def get_highest_word_score(word_list):
pass
highest_word_score = 0
winning_word = ""
words_len_ten = []

for word in word_list:
score = score_word(word)
if score == highest_word_score:
if len(word) == 10:
words_len_ten.append(word)
elif len(word) < len(winning_word) and not len(winning_word) == 10:
winning_word = word
elif score > highest_word_score:
highest_word_score = score
winning_word = word
if len(word) == 10:
words_len_ten.append(word)
Comment on lines +136 to +147
Copy link

Choose a reason for hiding this comment

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

Love this approach to tiebreak in a single pass over the input!


if words_len_ten:
return (words_len_ten[0], highest_word_score)
Comment on lines +149 to +150
Copy link

Choose a reason for hiding this comment

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

If we have several words tied for highest, then we should return the first of those high scores with 10 letters, if there is one. If we have input like ["AAAAAAAAAD", "JQ", "KFHK"] (In order, their scores are 11, 18, and 18), then we should still return JQ as the shortest of the highest scored words. The data in our tests didn't catch this scenario, but how could we update the function to account for it?

else:
return (winning_word, highest_word_score)
1 change: 1 addition & 0 deletions tests/test_wave_01.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ def test_letter_not_selected_too_many_times():
for i in range(1000):
# Arrange/Act
letters = draw_letters()
print(letters)
Copy link

Choose a reason for hiding this comment

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

We shouldn't commit changes to files that we don't intend to share out. It's all good to add code for testing or reformat things locally while we're working, but when we do, we should make sure that when we stage file for commit that we specify which files we are adding over using shortcuts that add all changed files. When we go to create a PR, we're also given a chance to review our changes before clicking "open" and that's a great place to double check that you only see the changes you intended to make.


letter_freq = {}
for letter in letters:
Expand Down