Skip to content

Sphinx - Vlada Rapaport #31

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Sphinx - Vlada Rapaport #31

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
142 changes: 138 additions & 4 deletions adagrams/game.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,145 @@

Copy link

Choose a reason for hiding this comment

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

Delete unneeded blank line

You can review the official Python style guide about blank line conventions.

import random

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.

letter_pool is a constant variable and should be named with capital letters like LETTER_POOL

'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 = ""
for letter , num in letter_pool.items():
letters += letter * num
Comment on lines +33 to +35
Copy link

Choose a reason for hiding this comment

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

Your implementation of draw_letters has 2 responsibilities: creating a pool of letters and also drawing random letters. Consider putting the logic on lines 33-35 into a helper function (maybe something like build_letter_pool and call that function here instead.


result = []
Copy link

Choose a reason for hiding this comment

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

The name result doesn't quite capture what this list represent in the game. Something like randomly_hand describes that this list is a random hand in the game which would make your code more self-documenting.

letters_count_dict = {
Copy link

Choose a reason for hiding this comment

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

The values of letters_count_dict will change so this is not a constant variable, you're correct to name this variable without using capital letters.

'A': 0,
'B': 0,
'C': 0,
'D': 0,
'E': 0,
'F': 0,
'G': 0,
'H': 0,
'I': 0,
'J': 0,
'K': 0,
'L': 0,
'M': 0,
'N': 0,
'O': 0,
'P': 0,
'Q': 0,
'R': 0,
'S': 0,
'T': 0,
'U': 0,
'V': 0,
'W': 0,
'X': 0,
'Y': 0,
'Z': 0
}

while len(result) < 10:
Copy link

Choose a reason for hiding this comment

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

I'd like to see the literal integer 10 referenced by a constant variable. As someone who might not know anything about this game, I might read line67 and wonder what the significance of 10 means.

How about something like:

HAND_SIZE = 10
while len(result) < HAND_SIZE:

i = random.randint(0, len(letters) - 1)
Copy link

Choose a reason for hiding this comment

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

What does i represent? Something like random_index would be more descriptive.

if letter_pool[letters[i]] > letters_count_dict[letters[i]]:
letters_count_dict[letters[i]] += 1
result.append(letters[i])

return result

def uses_available_letters(word, letter_bank):
pass
letter_bank_copy = []
word_lower = word.lower()
for letter in letter_bank:
letter_bank_copy.append(letter.lower())
Comment on lines +77 to +79
Copy link

Choose a reason for hiding this comment

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

Instead of having to call lower 2 times (on word and on each letter), you could call just upper on word since the letters in the letter_bank are already capitalized

Copy link

Choose a reason for hiding this comment

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

Instead of creating a variable to store the word with the case changed, you can directly call the function in the loop, like this:

for letter in word.lower()
    # rest of your code

Copy link

Choose a reason for hiding this comment

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

There are a couple of other ways to copy a list:

# use list comprehension
letter_bank_copy = [letter for letter in letter_bank]

# use string slicing
letter_bank_copy = letter_bank[:]


for letter in word_lower:
if letter not in letter_bank_copy:
return False
else:
letter_bank_copy.remove(letter)
return True
Comment on lines +81 to +86
Copy link

Choose a reason for hiding this comment

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

👍


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

Choose a reason for hiding this comment

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

score_dict should be a constant variable

'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
}
sum_word = 0
Copy link

Choose a reason for hiding this comment

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

sum_word doesn't quite convey what this variable represents in the game. A name like total_score would be more descriptive.

word_upper = word.upper()
for letter in word_upper:
Comment on lines +118 to +119
Copy link

Choose a reason for hiding this comment

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

Call upper directly in the for loop

sum_word += score_dict[letter]
if len(word) >=7 and len(word) <= 10:
sum_word += 8
Comment on lines +121 to +122
Copy link

@yangashley yangashley Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Using constant variables will make your code more self-documenting and easier to understand. If I don't know anything about this game, I'm left wondering what the significance of the integer literals 7, 10, and 8 are.

Consider something like:

BONUS_LENGTH_MIN = 7
BONUS_LENGTH_MAX = 10
BONUS_POINTS = 8

if BONUS_LENGTH_MIN <= len(word) <= BONUS_LENGTH_MAX:
        sum_word += BONUS_POINTS

return sum_word


def get_highest_word_score(word_list):
pass
max_word = word_list[0]
max_score = score_word(max_word)
for i in range(1, len(word_list)):
word = word_list[i]
score = score_word(word)
if score > max_score:
max_score = score
max_word = word
elif score == max_score and (len(word) != len(max_word)):
if len(word) == 10:
Copy link

Choose a reason for hiding this comment

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

We could use a constant variable to reference 10, maybe something like TIE_BREAKER_LENGTH or something else to describe why 10 is significant in this logic.

max_word = word
elif len(word) < len(max_word) and len(max_word) != 10:
max_word = word

return (max_word, max_score)
Copy link

Choose a reason for hiding this comment

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

👍 Nice job directly returning the tuple





Comment on lines +142 to +145
Copy link

Choose a reason for hiding this comment

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

Delete blank lines at end of file to keep project neat. Python recommends you keep one blank line so you can remove lines 143-145

You can check out this thread to read more if you'd like