Skip to content

Sphinx - Sarah Koch #25

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
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
165 changes: 161 additions & 4 deletions adagrams/game.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,168 @@
import random

NUM_DRAWN = 10

Choose a reason for hiding this comment

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

👍 I like to see this value referenced by a descriptively named variable


def draw_letters():
pass
'''
Creates a list of specific length. Elements of the list are randomly
selected letters from a pool defined within the function.

Returns:
letters_drawn (list): random letters in a list of X length

'''
letters_drawn = []
letter_pool = build_letter_pool()

for _ in range(NUM_DRAWN):
random_index = random.randint(0, len(letter_pool) - 1)
selected_letter = letter_pool[random_index]

letters_drawn.append(selected_letter)
letter_pool.remove(selected_letter)

return letters_drawn

def uses_available_letters(word, letter_bank):
pass
'''
Determines if a given word can be spelled using only letters from
the letter bank.

Checks whether each letter in the word is found in the letter bank,
removing letters as they are found.

Parameters:
word (str): The word to be checked.
letter_bank (list): List of letters available to form words.

Returns:
bool: True if the given word can be made using only the letters
available in letter_bank, otherwise False
'''
copied_list = []

for letter in letter_bank:
copied_list.append(letter)
Comment on lines +44 to +45

Choose a reason for hiding this comment

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

How would you use list comprehension to append the letters to copied_list?

You might also see the copy module used for making a new list by copying all the elements from an original list. Check out copy.deepcopy(list)


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

return True

def score_word(word):
pass
'''
Calculates the total score of the given word based on letter values.

Each letter has a specific assigned point value. The total score is
the sum of these values. Words of a minimum length receive an
additional bonus.

Parameters:
word (str): The word to be scored.

Returns:
total_score (int): The total score for the word, including any
applicable bonuses.
'''
POINT_VALUES = {

Choose a reason for hiding this comment

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

An extra white space slipped in between POINT_VALUES and =

'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
}
BONUS_LENGTH = 7
BONUS_POINTS = 8
Comment on lines +76 to +77

Choose a reason for hiding this comment

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

👍 Again, good work using constants for these values to make your code self-documenting.


total_score = sum(POINT_VALUES.get(letter.upper(), 0) for letter in word)

Choose a reason for hiding this comment

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

Nice and concise! What's going on in this line of code?

What's the data type of the object you pass to the sum function?

While the syntax on line 79 looks like list comprehension, it isn't.

It is interesting to think about the kind of iterable we should pass to a function like sum. For such a small data set like we have here it doesn't really matter, but have a look at this SO thread to read more about when we'd want to pass a list to sum instead.


if len(word) >= BONUS_LENGTH:
total_score += BONUS_POINTS
Comment on lines +81 to +82

Choose a reason for hiding this comment

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

While a hand can't be more than 10 letters and therefore a word can't be more than 10 letters, you could make this check more explicit and conform to the instructions in the README more by having the check on line 81 be: if BONUS_LENGTH <= len(word) <= 10:

And put that 10 in a constant too.


return total_score

def get_highest_word_score(word_list):
pass
'''
Finds and returns the highest-scoring word in the provided list of
words.

Compares the scores of each word and applies tie-breaking rules if
multiple words have the same score. Calls the tie_breaker helper
function if there is a tie. Returns the winning word and its
score.

Parameters:
word_list (list): A list of words to score and compare.

Returns:
tuple: Contains the highest-scoring word and its score.
'''
winning_word = None
highest_score = 0

for word in word_list:
word_score = score_word(word)

if winning_word is None or word_score > highest_score:
winning_word = word
highest_score = word_score
elif word_score == highest_score:
winning_word = tie_breaker(word, winning_word)

if is_max_length(winning_word):
break
Comment on lines +114 to +115

Choose a reason for hiding this comment

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

How would this logic work if you had a hand where there's a tie among ten letter words (maybe something like words = ["AAAAAAAAAA", "BBBBBBBBBB", "XXXXXXXXXX"]. Would this loop pick "XXXXXXXXXX" as the winning word?

How could you refactor your code to account for this scenario?


return (winning_word, highest_score)

def build_letter_pool():
'''
Builds and returns a list of letters repeated according to its
frequency as defined in the LETTER_POOL dictionary.

Returns:
list: A list of letters populated at their defined frequencies.
'''
LETTER_POOL = {

Choose a reason for hiding this comment

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

👍 Nice job naming this as a constant variable.

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

return [letter for letter, freq in LETTER_POOL.items() for _ in range(freq)]

Choose a reason for hiding this comment

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

I like the list comprehension here, it's Pythonic and concise! If someone asked you what this line of code was doing, how would you explain it?


def tie_breaker(word, current_winner):
'''
Determines the winning word in case of a tied score.

Preference is given to words of maximum length in their order of
appearance. If neither word is of maximum length, the word with the
fewest letters will be selected as the winner.

Parameters:
word (str): A word being compared for a tie.
current_winner (str): The current winning word.

Returns:
current_winner (str): The word that wins in tie-breaking logic.
'''
if is_max_length(current_winner):
return current_winner
Comment on lines +151 to +152

Choose a reason for hiding this comment

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

Do we need lines 151 and 152 if you go on to return current_winner anyways on line 156?

if is_max_length(word) or len(word) < len(current_winner):
return word

return current_winner

def is_max_length(word):
'''
Checks to see if the word has the maximum possible length.

Parameters:
word (str): The word to check.

Returns:
bool: True if the word is of max length, otherwise False.
'''
return len(word) == NUM_DRAWN
Comment on lines +158 to +168

Choose a reason for hiding this comment

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

Since this function contains a small amount of code len(word) == NUM_DRAWN I think it's ok to not have it in the helper function and just directly it on lines 151 and 153:

if len(current_winner) == NUM_DRAWN:
    return current_winner
if len(word) == NUM_DRAWN or len(word) < len(current_winner):
    return word

Having the helper does make tie_breaker a little more readable so maybe it's worth keeping in.

These kinds of comments will come up in a review. If I had a code review and got this comment, I'd either make the change to get rid of the helper function or I'd reach out to the reviewer and explain why I want to keep the helper function.

2 changes: 2 additions & 0 deletions main.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
from adagrams.ui_helper import *
from adagrams.game import draw_letters, uses_available_letters, score_word, get_highest_word_score

# First draft - no changes made yet

def wave_1_run_game():
display_welcome_message()
game_continue = True
Expand Down
2 changes: 1 addition & 1 deletion tests/test_wave_01.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ def test_letter_not_selected_too_many_times():

# Assert
for letter in letters:
assert letter_freq[letter] <= LETTER_POOL[letter]
assert letter_freq[letter] <= LETTER_POOL[letter]

Choose a reason for hiding this comment

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

Was this added white space an intentional change? It doesn't seem like it's needed, but just checking.

As a matter of keeping your PRs tidy, at internship, it's good practice PRs as concise as possible by only pushing up changes related to the feature changes being made.

Before I run git add/git commit/git push, I run git status and look at the files that were changed to make sure I didn't change accidentally change files besides the ones I intentionally wanted to change. Sometimes I'll also run git diff and do a quick skim to make sure my changes look as expected (harder to do with huge changes but that's another reason to make small frequent commits).

Anyways, this is all to say, if the white space on line 67 wasn't done on purpose then it's not a big deal to me that it was pushed up. However, if this was production code I'd tell you to remove it because we don't want to bring in accidental changes and checking in additional file makes you PR cluttered.


def test_draw_letters_returns_different_hands():
# Arrange/Act
Expand Down