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

C22/Natalie Sen/Adagrams Wave 1-4 #23

Open
wants to merge 1 commit into
base: main
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
142 changes: 138 additions & 4 deletions adagrams/game.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,145 @@
import random

def draw_letters():
pass
# define the dictionary with the letters and their value
LETTER_POOL = {

Choose a reason for hiding this comment

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

You are currently using the naming convention for a Global variable on LETTER_POOL (All caps) but placing it as a local variable. I would agree that this is should be a global variable, in which case make sure to move it outside the function!

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

def pick_random_letter():
# choose a letter randomly from the dict
letters = [letter for letter, value in LETTER_POOL.items() if value > 0] # looping through LETTER_POOL to get the value of the letters only if the value is bigger than 0
if not letters:
return None #if there are no available letters, return None
# use random.randint to pick a letter by index
index = random.randint(0, len(letters) - 1) # generates a random index between 0 and len(letters) - 1
return letters[index]

draw_letters = []

for i in range(10): #select exactly 10 letters from dict
letter = pick_random_letter()
if letter is None: # if None is returned indicating no more letters available, break the loop
break
draw_letters.append(letter) # adding the randomly picked letters into the initial empty list
LETTER_POOL[letter] -= 1 # decrease the count of the drawn letter in the dict
if LETTER_POOL[letter] == 0: # if the count of a letter reaches 0, removes the letter from the dict
del LETTER_POOL[letter] # remove the letter if its value reaches 0

return draw_letters
Comment on lines +34 to +54

Choose a reason for hiding this comment

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

There is some really cool stuff in this function/algorithm that you wrote. The nested function is well written and can be useful in a variety of situations. Typically however, we'll want to use a nested function only if it makes sense and I don't think this is one of those situations. Your pick_random_letter() function is technically doing what you want it, but you are making certain checks that we don't actually need to make! In a sense, you are overcomplicating your code here.

One place where I think there's a bit of confusion is how you are handling LETTER_POOL. It is true that you are destroying LETTER_POOL as you remove the tiles that get added to your hand, so you might potentially need to make checks to see if you have enough tiles left. My concern with this particular nested function is that it recreates the list comprehension on line 36 every time it gets called. It would be much better if we had a more direct way of figuring out our range for randint.

I think that overall, the best way to refactor this would be to use LETTER_POOL to make a list of every single potential letter. From there, you can run your while loop. Within the while loop, use randint to select single letters from the newly created list. Because that list is essentially a copy, we can remove the letters and essentially destroy the list without worrying about needing to use it again later.


result = draw_letters()

def uses_available_letters(word, letter_bank):
pass
# function that counts each letters that occured in the list of letters
# this part of the code can be replaced by import counter
def count_letters(letters):

Choose a reason for hiding this comment

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

I love the idea of a helper function here to make dictionaries out of a word count! My one concern is that it is currently a nested function. Typically we only nest functions if they only apply to the function they are being defined in. This is one that could actually be pretty useful elsewhere or in general, so I would make it it's own function rather than nesting it!

counts = {}
for letter in letters:
if letter in counts:
counts[letter] += 1
else:
counts[letter] = 1
return counts

# convert word and letter_bank to lowercase to handle case sensitiveness
word = word.lower()
letter_bank = [letter.lower() for letter in letter_bank]

Choose a reason for hiding this comment

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

Great list comprehension!


# count the frequencies of the letters in word and letter_bank (word represents the user input words and letter_bank represent the letters that have been drawn)
word_counts = count_letters(word)
letter_bank_counts = count_letters(letter_bank)

# check to see if every letters in word is available in letter_bank
for letter, count in word_counts.items():
if letter_bank_counts.get(letter, 0) < count:
return False # if the letter count in word is more than what we have in letter_bank
return True # if all the letters are available in letter_bank

Choose a reason for hiding this comment

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

The rest of this function is super clean! Well done!



def score_word(word):
pass
# score map, value of each letters
letter_scores = {

Choose a reason for hiding this comment

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

This dictionary looks good! Just a heads up that it could also be seen as a global variable that could be used elsewhere!

'A': 1, 'E': 1, 'I': 1, 'O': 1, 'U': 1,
'L': 1, 'N': 1, 'R': 1, 'S': 1, 'T': 1,
'D': 2, 'G': 2,
'B': 3, 'C': 3, 'M': 3, 'P': 3,
'F': 4, 'H': 4, 'V': 4, 'W': 4, 'Y': 4,
'K': 5,
'J': 8, 'X': 8,
'Q': 10, 'Z': 10
}
# convert the word to uppercase to match the key in letter_score
word = word.upper()
# initialize base score
base_score = 0
for letter in word:
base_score += letter_scores.get(letter, 0)

# calculate the bonus points for the words od length 7 or more
if len(word) >= 7:

Choose a reason for hiding this comment

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

Don't forget that we don't add 8 if the word goes over 10 letters! That's a similar condition!

bonus_points = 8
else:
bonus_points = 0

total_score = base_score + bonus_points

Choose a reason for hiding this comment

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

As you get more comfortable with python, I would encourage you to keep the score to a single variable for the entirety of the function! It helps streamline things just a bit! This would replace the instinct to separate out the score into base and bonus and just keep it all in one place and add it up as the function progresses.


return total_score


def get_highest_word_score(word_list):
pass
highest_word = ""
highest_score = 0

# loop through the word_list to get the score
for word in word_list:
score = score_word(word)

if score > highest_score:
highest_word = word
highest_score = score
# check if current word's score is equal to the highest score.
elif score == highest_score:
# check if the current word has 10 characters, it will replacw the previous highest word
if len(word) == 10 and len(highest_word) != 10:
highest_word = word
# if the current word has less characters it will replace the previous highest word
elif len(word) < len(highest_word) and len(highest_word) != 10:
highest_word = word
# assign current word to highest word if the lengths of the current word and highest word are the same
elif len(word) == len(highest_word) and highest_word == "":

Choose a reason for hiding this comment

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

What the comment says this if statement is doing is very different than what the if statement is actually doing! You've included a check to see if highest word doesn't actually exist. The only way this would ever be triggered is if the list is just empty strings, which we know isn't the case, so this final elif is actually redundant. Other than that, the logic you have here is great!

highest_word = word

return (highest_word, highest_score)

Choose a reason for hiding this comment

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

Just a small thing here, but we don't actually need the parentheses. If we just return two values, the return statement will tupelize (definitely not a word, but I couldn't resist) the returns by default!