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

Panthers - Jamaica A. #73

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
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
131 changes: 127 additions & 4 deletions adagrams/game.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,134 @@
import random

LETTER_POOL = {

Choose a reason for hiding this comment

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

Since LETTER_POOL and score_chart are each 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.


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



score_chart = {

Choose a reason for hiding this comment

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

Placed out here as a global constant, I would recommend using all caps naming: SCORE_CHART.

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

# Wave 1

def draw_letters():
pass
new_list =[]

while len(new_list) <10:
letter = (random.choices(list(LETTER_POOL.keys()), weights = list(LETTER_POOL.values()), k=1))[0]
Comment on lines +69 to +70

Choose a reason for hiding this comment

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

Nice use of random.choices!

This line gets pretty long, to keep it under the PEP8 recommendation of 79 characters we should split it up over a couple lines. Something we could do that would also lighten the processing we do each iteration could be pulling the list creations out of this line and storing them in variables above the loop.

    letters_list = list(LETTER_POOL.keys())
    letter_weights = list(LETTER_POOL.values())

    while len(new_list) <10:
        letter = random.choices(letters_list, weights = letter_weights, k = 1)[0]

if new_list.count(letter) < LETTER_POOL[letter]:
new_list.append(letter)

return(new_list)

Choose a reason for hiding this comment

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

We want to be consistent in how we do things across a project and remove syntax we don't need. I'd recommend always placing a space between the return keyword and the content that follows, and removing surrounding parentheses unless they are required to separate multiple expressions.


# Wave 2

def uses_available_letters(word, letter_bank):
pass
letter_bank_copy = letter_bank.copy()
if len(word) > len(letter_bank_copy):
return False
Comment on lines +79 to +81

Choose a reason for hiding this comment

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

Nice quick check to rule out words with an invalid length!

This implementation will allocate space for letter_bank_copy before we know if the input word is too long to be valid. If line 80 checked against the length of the input letter_bank, then we could move the if statement to the top of the function, and only create the deep copy if we know we need it.

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

    letter_bank_copy = letter_bank.copy()

for letter in word.upper():
if letter in letter_bank_copy:
letter_bank_copy.remove(letter)
else:
return False

Choose a reason for hiding this comment

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

Nice early exit as soon as we find an invalid character!


return True

# Wave 3

def score_word(word):
pass

score = 0
if 7 <= len(word) <= 10:

Choose a reason for hiding this comment

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

Nice chaining of comparison operators!

score += 8
for letter in word.upper():
if letter.isalpha():
score += score_chart.get(letter)
Comment on lines +98 to +99

Choose a reason for hiding this comment

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

isalpha will return True if it can determine that all the characters in the string are alphabet characters. It isn't limited to the Latin A-Z character set. If a user submitted a string with, as some examples, Japanese characters or Cyrillic characters, we would likely get a KeyError on line 99. It means making more searches through the score_chart, but we could prevent this by checking if the character is a member of the keys in score_chart before trying to access it:

# Using `get` with `None` as the value for missing keys
for letter in word.upper():
    value = score_chart.get(letter, None)
    if value:
        score += value
    # We could also use python's version of a ternary operator to combine the last 2 lines
    # The commented code below could be read as: 
    # "score is increased by `value` if `value` is truthy, otherwise increase score by 0"
    # score += value if value else 0

# Using `in`
for letter in word.upper():
    if letter in score_chart:
        score += score_chart[letter]

return score

# Wave 4

def get_highest_word_score(word_list):
pass

top_score = 0
winner_list = []


for i in range(len(word_list)):
word = word_list[i]
score = score_word(word_list[i])
if score_word(word) > top_score:
top_score = score
Comment on lines +111 to +114

Choose a reason for hiding this comment

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

If we declare a variable, we want be sure to use it so we can avoid calling functions multiple times if we don't need to:

        word = word_list[i]
        score = score_word(word) 
        if score > top_score:
            top_score = score 


for i in range(len(word_list)):
if score_word(word_list[i]) == top_score:
winner_list.append(word_list[i])
Comment on lines +106 to +118

Choose a reason for hiding this comment

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

If we were in a system where we needed to reduce loops over the input, we could combine these loops into one:

    top_score = 0
    winner_list = []

    for word in word_list:
        score = score_word(word)

        if score > top_score:
            top_score = score
            winner_list = [word]

        elif score == top_score:
            winner_list.append(word)


if len(winner_list) == 1:
winner = (winner_list[0])

Choose a reason for hiding this comment

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

Great thought to check the length of winner_list to see if we can exit early!


if len(winner_list) > 1:

Choose a reason for hiding this comment

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

Since we know we will never need to enter the if here on line 123 if we entered if len(winner_list) == 1: on line 120, I would consider using an elif here. With an elif, if we hit line 120 and enter if len(winner_list) == 1:, then the code knows we entered the if-statement and will not bother evaluating the following connected elif statements, saving us the tiniest bit of processing.

winner1 = max(winner_list, key=len)
winner2 = min(winner_list, key=len)
Comment on lines +124 to +125

Choose a reason for hiding this comment

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

winner1 and winner2 as names don't tell us much about the difference between these variables. I would consider using names that give the reader more information. One of many possibilities could be longest_winner & shortest_winner.

if len(winner1) == 10:
winner = winner1
else:
winner = winner2
Comment on lines +126 to +129

Choose a reason for hiding this comment

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

Really nice use of min and max to tiebreak on length!


winner_score = score_word(winner)

return(winner, winner_score)
Comment on lines +131 to +133

Choose a reason for hiding this comment

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

The variable top_score is already holding the max score for our tied words. This means we could remove line 131 and use top_score on line 133:

    return(winner, top_score)