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

Spruce - Asha P. #43

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Spruce - Asha P. #43

Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
061de8d
Adds flag before for loop letter not found and in while loop whenlett…
GabeKelemen Sep 22, 2021
c83375e
refactor uses_available_letters(word, letter_bank) function to pass …
ashapa Sep 23, 2021
12e7020
updates draw_letters() with reordered logic and boolean flag
GabeKelemen Sep 23, 2021
0d4ea0a
Merge branch 'Ada-C16:master' into master
GabeKelemen Sep 23, 2021
1907bd0
Merge branch 'master' of https://github.com/ashapa/adagrams-py
GabeKelemen Sep 23, 2021
3126b6f
updates draw_letters
GabeKelemen Sep 23, 2021
610b8c2
adds uses_available_letters function
GabeKelemen Sep 23, 2021
748787c
updates score_word(), tests passed
GabeKelemen Sep 23, 2021
febc543
finished 4 waves, tests passed
GabeKelemen Sep 25, 2021
484b84a
make edits to document formatting for adagrams
ashapa Sep 25, 2021
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
139 changes: 135 additions & 4 deletions adagrams/game.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,142 @@
import random
import copy


LETTER_POOL = {
Copy link
Collaborator

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. 🙂

'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

temp_letter_pool = copy.deepcopy(LETTER_POOL)
Copy link
Collaborator

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_list_from_pool = list(LETTER_POOL.keys())
letter_not_found = True
letters_drawn_list = []

letter = ""
Copy link
Collaborator

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).


for draw in range(10):
Copy link
Collaborator

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
Comment on lines +46 to +59
Copy link
Collaborator

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


return letters_drawn_list


def uses_available_letters(word, letter_bank):
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Awesome use of a frequency map to figure out how many of each letter is available, and then consuming the available tiles while iterating over the word!

pass
letter_bank_letter_count = {}
for letter in letter_bank:
if letter in letter_bank_letter_count:
letter_bank_letter_count[letter] += 1
else:
letter_bank_letter_count[letter] = 1
Comment on lines +67 to +70
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use the get method of a dictionary with a default value lets us simplify the counting up for the map a little bit:

        letter_count = letter_bank_letter_count.get(letter, 0)
        letter_bank_letter_count[letter] = letter_count + 1

Alternatively, take a look at the collections Counter helper.


for letter in word:
# if a letter in word is in the letter_bank,
# then remove one of the counts from the letter tally in the dict
# to signify that letter in the letter_bank has been taken
if letter in letter_bank_letter_count and letter_bank_letter_count[letter] > 0:
letter_bank_letter_count[letter] -= 1
else:
return False
Comment on lines +76 to +79
Copy link
Collaborator

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


return True


def score_word(word):
pass
word = word.lower()

SCORE_CHART = {
Copy link
Collaborator

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.

'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': 6,
'o': 1,
'p': 3,
'q': 10,
'r': 1,
's': 1,
't': 1,
'u': 1,
'v': 4,
'w': 4,
'x': 8,
'y': 4,
'z': 10
}

word_score = 0
for letter in word:
word_score += SCORE_CHART[letter]
if 6 < len(word) < 11:
word_score += 8
Comment on lines +119 to +120
Copy link
Collaborator

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?

return word_score


def get_highest_word_score(word_list):
pass

# this is the comparison, set to the value of the first in list
winning_word = word_list[0]
Copy link
Collaborator

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?


for word in word_list:
if score_word(word) > score_word(winning_word):
winning_word = word
elif score_word(word) == score_word(winning_word):
# if new word is 10 letters and old word isn't 10 letters, replace
if len(word) >= 10 and len(winning_word) < 10:
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
Comment on lines +134 to +138
Copy link
Collaborator

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?


best_word = (winning_word, score_word(winning_word))

return(best_word)