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 - Cecily M. and Angela F. #28

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
+113 −4
Open

Spruce - Cecily M. and Angela F. #28

Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
663254a
wave 1 pseudo
cecily-916 Sep 21, 2021
773282a
draw letters update test
fan-gela Sep 22, 2021
6f3b604
update function uses_available_letters
cecily-916 Sep 22, 2021
b28f745
conflict resolved first two functions
cecily-916 Sep 22, 2021
b500d90
update first function
cecily-916 Sep 22, 2021
ace0c1a
score_words complete
fan-gela Sep 23, 2021
3c59070
merged
fan-gela Sep 23, 2021
b58b0ad
update get_highest_word_score
cecily-916 Sep 23, 2021
14d3289
update get_highest_word_score
cecily-916 Sep 23, 2021
01a02dc
refactored get_highest_word_score
cecily-916 Sep 23, 2021
3be533f
Merge branch 'master' of https://github.com/cecily-916/adagrams-py
cecily-916 Sep 23, 2021
e2f2c9f
submitting final version
cecily-916 Sep 23, 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
117 changes: 113 additions & 4 deletions adagrams/game.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,120 @@
import random

def draw_letters():
pass
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.

Consider declaring this as a "constant" (maybe POOL_OF_LETTERS) outside this function. Doing so wouldn't really save on runtime (since we need to modify the counts, we'd need to make a copy at the start of each call to the method), but it does declutter the method a bit by allow us to move the large data structure to some innocuous corner of our file.

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

letters_drawn = []
letter_pool_list = []

for key in letter_pool:
for idx in range(letter_pool[key]):
letter_pool_list.append(key)
Comment on lines +36 to +38
Copy link
Collaborator

@anselrognlie anselrognlie Oct 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This code is building a list of expanded letters to be used for random picking later. From a purely syntax perspective, keep in mind that if we know that we'll need both the key and the value (letter_pool[key] here), we can iterate using the .items() helper to get both at once. Further, we can use list repetition to avoid the inner loop. Combined, this would look something like:

    for letter, count in letter_pool.items():
        letter_pool_list += [letter] * count

The alternative approach would be to make a copy of the letter count data, and pick random letters, decrementing their available counts. The initial copy (required so that the next draw has all the letters available again) is roughly equivalent in time to building the extra letter list here. The upside would be that each letter pick would be roughly O(1) rather than O(n) for operations like the choice and remove operations.


while len(letters_drawn) != 10:
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 logic here guarantees that a random letter will be successfully drawn each time through the loop, we could use a for loop with a range expression to run this 10 times. Other approaches (such as the decrement approach I mentioned above) might have loops where a valid letter isn't chosen (if the count is 0), in which case while loop based on the accumulated length of the selected tile list is also a great approach. But here, we could do:

    for _ in range(10):

The use of _ as the variable name is usually a signal to developers that we don't care about this variable (it isn't used), but one is required for syntax reasons.

random_entry = random.choice(letter_pool_list)
letters_drawn.append(random_entry)
letter_pool_list.remove(random_entry)
return letters_drawn

def uses_available_letters(word, letter_bank):
pass

letters_copy = letter_bank.copy()
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 approach used to check whether the word can be made from the letter bank is destructive to the hand, this copy is necessary (otherwise we'd be destroying the player's hand anytime we check for word validity!).

Another approach would be to build a frequency map out of the letter bank, and then check whether each letter in word can be accounted for in the map. This would have the upside that each letter check would be O(1), while checking for in in a list (and the subsequent remove) is O(n) for each letter.

word_length = len(word)

try:
while word_length > 0:
for letter in word:
if letter in letters_copy:
letters_copy.remove(letter)
word_length -= 1
else:
raise ValueError
return True
Comment on lines +52 to +59
Copy link
Collaborator

@anselrognlie anselrognlie Oct 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Nice use of errors!

Notice, however, that remove itself will raise a ValueError if we attempt to remove an item that isn't in the list, so we don't need to do the in check and raise the error ourselves. We also don't need to track the word length ourselves, since if we exhaust the available letters in letters_copy before finishing iterating over the letters in word, we'll end up in the error handler, but if there were enough letters to satisfy the word, we'll reach the end of the word and return True. Something like the following:

        for letter in word:
            letters_copy.remove(letter)
        return True

except ValueError:
return False

def score_word(word):
pass

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 in draw_letters, 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.

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

word = word.upper()
total_points = 0

for letter in word:
total_points += score_chart[letter]
if len(word) >= 7 and len(word) <= 10:
total_points += 8
Comment on lines +99 to +100
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 = 7
MAX_BONUS_LENGTH = 10
LENGTH_BONUS = 8

Something else to consider, is it even necessary to check whether the length of the word is less than or equal to 10?


return total_points

def get_highest_word_score(word_list):
pass

highest_scoring_word = ("",0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider keeping these as 2 distinct variables during the calculation, and wait to return them together until the end of the method. By working with a tuple directly here, we need to keep track of which position in the tuple holds the score, and which holds the word.

Instead, we could make 2 variables: highest_scoring_word and highest_score, so then we would be very clear about which value we were working with. Then, at the end, we could return highest_scoring_word, highest_score.


for word in word_list:
score = score_word(word)
if score > highest_scoring_word[1]:
highest_scoring_word = (word, score)
elif score == highest_scoring_word[1]:
if len(word) == len(highest_scoring_word[0]) \
or len(highest_scoring_word[0]) >= 10:
Comment on lines +113 to +114
Copy link
Collaborator

Choose a reason for hiding this comment

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

PEP8 recommends that we avoid explicit line breaks (\) and instead prefer implicit line breaks (any expression surrounded by parentheses can pretty much be wrapped anywhere). For example:

            if (len(word) == len(highest_scoring_word[0])
                or len(highest_scoring_word[0]) >= 10):

continue
if len(word) >= 10 \
or len(word)< len(highest_scoring_word[0]):
highest_scoring_word = (word, score)

return highest_scoring_word