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

Snow Leopards C18 -Geiselle H. and Bukunmi G. #77

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

Snow Leopards C18 -Geiselle H. and Bukunmi G. #77

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
89922fc
Wave 1 attempt 1 Geiselle
geiselleholt Sep 28, 2022
ed7aa2a
Wave 1 all test passed
geiselleholt Sep 28, 2022
517d8b4
Wave 2 passed all tests
geiselleholt Sep 28, 2022
47818e0
Wave 3 all tests passed
geiselleholt Sep 28, 2022
84bd7d0
Wave 4 attempt 1
geiselleholt Sep 29, 2022
d95609a
Wave 4 all tests passed
geiselleholt Sep 29, 2022
14061ae
Refactoring
geiselleholt Sep 30, 2022
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
116 changes: 112 additions & 4 deletions adagrams/game.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,119 @@
import string
from random import sample
from collections import Counter

def draw_letters():
pass

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 you used sample to draw your hand (which is non-destructive), this definition of LETTER_POOL could be moved outside the function to be global (to the module). This allows us to focus on the logic within the function (which is a single line in this case), rather than having the body of the function be dominated by the data definition.

Copy link

Choose a reason for hiding this comment

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

When using strings as the dictionary keys, I like to the use the dict() function-stlye of making a dictionary. It uses keyword argument calling to let us set string keys without needing to quote them. For example

    LETTER_POOL = dict(
        A=9,
        B=2,
        C=2,
        ...
    )

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

letter_bank = sample(LETTER_POOL.keys(), k=10, counts=LETTER_POOL.values())
Copy link

Choose a reason for hiding this comment

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

Really nice, concise approach. When bringing in functions from imported modules (that we haven't covered in the curriculum), please consider including a comment about how you understand the function to work, and how you reasearched it. Now that we've discussed Big O, it's also useful to think about what the time/space complexity of the function might be. Thinking about how we can implement this functionality ourselves can help us gain insight into what that might be.

Remember that not every language has the same built-in functions, so we should be able to implement this same functionality ourselves. In the projects, we don't ask for any features that we don't think you should be able to write yourself. For drawing a random hand, the only external function that would be "required" is a way to pick a random number (such as randint). At this stage in our coding journeys, it's often more valuable to re-implement things by hand to practice thinking about how to breakdown and approach the problem than to use a single library function to solve the problem for us.

Be careful to note that the way this is being called is resulting in a warning in the tests:

tests/test_wave_01.py: 1002 warnings
  adagrams/game.py:36: DeprecationWarning: Sampling from a set deprecated
  since Python 3.9 and will be removed in a subsequent version.
    letter_bank = sample(LETTER_POOL.keys(), k=10, counts=LETTER_POOL.values())


return letter_bank


def uses_available_letters(word, letter_bank):
pass

word = word.upper()

for letter in word:
if letter not in letter_bank:
return False
Comment on lines +45 to +47
Copy link

Choose a reason for hiding this comment

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

This handles the case of checking for word letters that aren't in the bank at all. Note that the not in conceals a hidden loop, since we can only check whether something isn't in a list by checking every value in the list. Doing that in a loop can start us down the road towards quadratic time complexity. For adagrams, we do have some bounds on the data size that make this less of a concern, but think about the general case where the letter bank might have n tiles, and the word might be up to m letters long. If we assume that in the worst case, the word could have the same number of letters as the hand has tiles, this truly does approach O(n^2) time complexity.

Could this checking be combined with the general checking being done later on?


count_word = Counter(word)
count_letter_bank = Counter(letter_bank)

for k, v in count_word.items():
Copy link

Choose a reason for hiding this comment

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

Consider more descriptive variable names. Maybe word_letter and word_letter_count?

if k in count_letter_bank:
Copy link

Choose a reason for hiding this comment

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

Here as well, bank_letter?

if v <= count_letter_bank[k]:
return True
Comment on lines +54 to +55
Copy link

Choose a reason for hiding this comment

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

The sense of this comparison is backwards. This check will report that a word can be made as soon as it finds a letter that has a count in the word that isn't greater than the count in the bank. Consider the case where our hand contains A, B, C, ..., J, and the "word" is ABB. Since there is an A in the hand, this check will return True, even though there are not enough Bs in the hand to make the word.

This check should instead look for the word letter count being larger than the bank letter count, returning False (can't make this word) in that case. Then if we reach the end of the function, that means we didn't find any letters that violated their counts, so we can make the word and return True.

    for k, v in count_word.items():
        if k in count_letter_bank:
            if v > count_letter_bank[k]:
                return False

    return True


return False


def score_word(word):
pass
LETTERS_VALUES = {
Copy link

Choose a reason for hiding this comment

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

This data structure could also be pulled out of the function into the global space of the module.

"A" : 1,
"E" : 1,
"I" : 1,
"O" : 1,
"U" : 1,
"L" : 1,
"N" : 1,
"R" : 1,
"S" : 1,
"T" : 1,
"D" : 2,
"G" : 2,
"B" : 3,
"C" : 3,
"M" : 3,
"P" : 3,
"F" : 4,
"H" : 4,
"V" : 4,
"W" : 4,
"Y" : 4,
"K" : 5,
"J" : 8,
"X" : 8,
"Q" : 10,
"Z" : 10,
}
word = word.upper()
word_value = 0
for letter in word:
if letter in LETTERS_VALUES:
word_value += LETTERS_VALUES[letter]
if len(word) > 6:
word_value += 8
Comment on lines +91 to +95
Copy link

Choose a reason for hiding this comment

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

👍 Nice job tallying up the score and adding the bonus.

You did a check to see whether a letter was in score table. This is fine and has the affect that any unexpected tiles in the word don't contribute to the score. But where would those unexpected tiles have come from? And should it be the scoring function's job to handle those? We might expect some other part of the game to validate that any words played by a player include only tiles from the tile bank, in which case this function could safely ignore the in check. In fact, we might want to let python raise a KeyError so that the program can find out that, somehow, bad data got into our word.

return word_value

def get_highest_word_score(word_list):
pass

score_list = []
for word in word_list:
score = score_word(word)
score_list.append(score)

word_score_tuples = list(zip(word_list, score_list))
Comment on lines +100 to +105
Copy link

Choose a reason for hiding this comment

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

This would be a great opportunity for a list comprehension:

    word_score_tuples = [(word, score_word(word)) for word in word_list]


sorted_tuples = sorted(word_score_tuples, key=lambda t: -t[1])
Copy link

Choose a reason for hiding this comment

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

This will sort the word/score tuples in descending order, leaving the highest scores at the front of the list. Recall that sort/sorted must be considered O(n log n). So while this can be a step in how we find the winning word, keep in mind that there are O(n) (linear) time complexity approaches as well.

winning_word = sorted_tuples[0]
if sorted_tuples[0][1] == sorted_tuples[1][1]:
Copy link

Choose a reason for hiding this comment

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

This works for the test data, because there are never more than two words that are tied for the hieghest score, but this would not work if there were 3 or more words tied (e.g., ["BBBBBB", "BBBBBB", "ZX"]). This would also fail if there were only a single word in the word list (solitaire adagrams).

While passing all the tests is great, the tests are not (and can never truly be) exhaustive. Be sure you understand the general situation a test is looking for, and write your code to handle that general case, not just the specific case of the particular test. You can always add your own additional tests to check that your implementation is robust to a wider variety of data.

How could we modify this approach so that we process as many words as necessary (2, 3, 4, ...), and don't crash on a single word.

if len(sorted_tuples[0][0]) == 10:
Copy link

Choose a reason for hiding this comment

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

One drawback to packing the words and scores into tuples from the beginning is that we have a lot of indexing in the calculations, which can be difficult to read. We could make some constants to make things a little more readable

    # define earlier in code
    WORD = 0
    SCORE = 1

    # then we can write
    sorted_tuples[0][WORD] # to get the word from the tuple
    sorted_tuples[0][SCORE] # to get the score from the tuple

winning_word = sorted_tuples[0]
elif len(sorted_tuples[1][0]) == 10:
winning_word = sorted_tuples[1]
elif len(sorted_tuples[0][0]) > len(sorted_tuples[1][0]):
winning_word = sorted_tuples[1]
elif len(sorted_tuples[0][0]) < len(sorted_tuples[1][0]):
winning_word = sorted_tuples[0]

return winning_word
2 changes: 1 addition & 1 deletion main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ def wave_1_run_game():
display_retry_instructions()
continue_input = input()
game_continue = continue_input == "y"

display_goodbye_message()

def wave_2_run_game():
Expand Down