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

Heather M. & Esther A. adagrams project #52

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open
129 changes: 125 additions & 4 deletions adagrams/game.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,132 @@
# from tests.test_wave_01 import LETTER_POOL

Choose a reason for hiding this comment

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

We want to remove commented code from files when we're done testing.

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 is only used by the draw_letters function, it would be reasonable to place this inside the function rather than as a constant. There are tradeoffs, this structure does clutter the function some, but it would keep the data as close as possible to where it's being used, and would mean other functions would have no way to 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.

The feedback on line 3 about placing data in the function where its used applies here too.

("A", "E", "I", "O", "U", "L", "N", "R", "S", "T") : 1,

Choose a reason for hiding this comment

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

Since they are iterable and non-mutable, we could also use strings instead of tuples here:

Suggested change
("A", "E", "I", "O", "U", "L", "N", "R", "S", "T") : 1,
"AEIOULNRST" : 1,

("D", "G") : 2,
("B", "C", "M", "P") : 3,
("F", "H", "V", "W", "Y") : 4,
("K") : 5,
("J","X") : 8,
("Q","Z") : 10,
}


def draw_letters():
pass
'''
Creates a new randomized list of letters from LETTER_POOL.
Returns: A randomized list of ten letters.
'''

letters = [letter for letter, letter_frequency in LETTER_POOL.items() for number in range(letter_frequency)]

Choose a reason for hiding this comment

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

This is a really cool solution to condense filling the letters list to a single line! I would suggest breaking this apart to make it easier to read and understand. The PEP 8 style guide recommends limiting line to a max of 79 characters to make code easier to read, especially with multiple editor windows open: https://peps.python.org/pep-0008/#maximum-line-length. Outside of the style guide, when you're in situations where other people will be reading and working in the same code, the ability to quickly and easily comprehend what the code is doing is often more valuable than reducing line count. For list comprehensions like this, I would strongly suggest including a comment that describes what the line is doing.


random.shuffle(letters)

return letters[:10]

def uses_available_letters(word, letter_bank):
pass
'''
Checks that each letter is listed in the dictionary as >=
as used in the word.

Parameters: string, dict (word and letter frequency)
Returns a Boolean value representing the validity of the input.
'''

word_uppercase = word.upper()

for letter in word_uppercase:
if letter not in letter_bank or (word_uppercase.count(letter) > letter_bank.count(letter)):

Choose a reason for hiding this comment

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

This is another line I'd recommend breaking for length:

Suggested change
if letter not in letter_bank or (word_uppercase.count(letter) > letter_bank.count(letter)):
letter_count_word = word_uppercase.count(letter)
letter_count_bank = letter_bank.count(letter)
if letter not in letter_bank or (letter_count_word > letter_count_bank):

return False

return True


def score_word(word):
pass
'''
Score is given to each letter based on the score chart dictionary.
If the word is between 7 and 10, add 8 points to score.

Parameters: A string of the word.

Returns the score of the word.
'''
score = 0
word_uppercase = word.upper()
if len(word_uppercase) >= 7 and len(word_uppercase) <= 10:

Choose a reason for hiding this comment

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

We could rewrite this to call len once with chained comparison operators:

Suggested change
if len(word_uppercase) >= 7 and len(word_uppercase) <= 10:
if 7 <= len(word_uppercase) <= 10:

Some resources on chaining comparison operators if y'all are interested:
https://docs.python.org/3/reference/expressions.html#comparisons
https://www.geeksforgeeks.org/chaining-comparison-operators-python/

Copy link
Author

Choose a reason for hiding this comment

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

Hey, Kelsey! I was actually really struggling with this one. I initially wrote it exactly as you suggested, but it wasn't working. I still can't figure out why.

Copy link

@kelsey-steven-ada kelsey-steven-ada Apr 7, 2022

Choose a reason for hiding this comment

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

Was there an error, or did the logic produce the wrong result? I'd be curious what you see stepping through with the debugger.

Update: I did a quick test pulling down the code and changing just this line, and all tests are passing on my machine. If you do the same and hit an issue, could you share what you're seeing?

score += 8

Choose a reason for hiding this comment

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

I'd consider adding a new line after this statement to have visual space between the block of code that handles adding the length bonus and the code that tallies up the letter score.

for letter in word_uppercase:
for letter_freq in SCORE_CHART:
if letter in letter_freq:
score += SCORE_CHART[letter_freq]
Comment on lines +86 to +89

Choose a reason for hiding this comment

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

This is a solid solution! Another approach would be to hold the scores in a dict that looked like LETTER_POOL with individual letters as keys and scores as values. We'd be trading off having another long data structure declaration and duplicating point values to simplify the loop a little:

Suggested change
for letter in word_uppercase:
for letter_freq in SCORE_CHART:
if letter in letter_freq:
score += SCORE_CHART[letter_freq]
for letter in word_uppercase:
score += SCORE_CHART[letter]

return score


def get_index_tie_break(word_list):

Choose a reason for hiding this comment

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

Great use of a helper function!

'''
Checks if the length of the word is 10, then checks for the first word with the fewest letters
Otherwise, returns the first word.

Parameter: List of highest scoring, tied words.

Return the index of the winning word.
'''
highest_index = 0

Choose a reason for hiding this comment

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

highest_index feels a little ambiguous in meaning to me here. Something along the lines of highest_scored_index or winning_index might describe the contents a little closer.

for i in range(len(word_list)):
if len(word_list[i]) == 10:
return i
elif len(word_list[i]) < len(word_list[highest_index]):
highest_index = i
return highest_index
Comment on lines +102 to +108

Choose a reason for hiding this comment

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

Another approach would be to loop directly over the word_list and return the winning word itself:

Suggested change
highest_index = 0
for i in range(len(word_list)):
if len(word_list[i]) == 10:
return i
elif len(word_list[i]) < len(word_list[highest_index]):
highest_index = i
return highest_index
shortest_word = None
for word in word_list:
if len(word) == 10:
return word
elif not shortest_word or len(word) < len(shortest_word):
shortest_word = word
return shortest_word



def get_highest_word_score(word_list):
pass
'''
Calculate high score for each word using score_word and calculate the winner if there is a
tie using get_index_tie_break().

Parameter: A list of words.

Returns a tuple of the highest scoring word and its high score.
'''
highest_scoring_words = []
high_score = 0
high_score_index = 0

Choose a reason for hiding this comment

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

I recommend moving this initialization down to right above if len(highest_scoring_words) > 1: on line 130 to keep the creation of this data close to where it is used.

for word in word_list:
score = score_word(word)
if score > high_score:
highest_scoring_words = [word]
high_score = score
elif score == high_score:
highest_scoring_words.append(word)
if len(highest_scoring_words) > 1:

Choose a reason for hiding this comment

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

I'd recommend whitespace between the for loop that finds the max scored words and the if statement that handles tie breaks & returns to help folks see the logical sections of the function at a glance.

high_score_index = get_index_tie_break(highest_scoring_words)
return highest_scoring_words[high_score_index], high_score

Choose a reason for hiding this comment

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

I like how you're using lists where you need to build up data then converting it to a tuple right when you need it.

2 changes: 1 addition & 1 deletion main.py
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