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

Otters - Amel N - Mitra M #44

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

mitramagh
Copy link

No description provided.

Copy link

@goeunpark goeunpark left a comment

Choose a reason for hiding this comment

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

Great job, Amel & Mitra! 🎉

Your tests pass, your commit messages are descriptive and frequent, and your code is crystal clear! 🔮 I'm impressed with the subtle styling in this submission, and it's clear to me that y'all are comfortable with working with lists, dictionaries, and tuples! This project is a Green. 🟢

Keep up the great work!


# Draw a random letter
while len(letters) < 10:
letter = random.choice(list(LETTER_POOL))

Choose a reason for hiding this comment

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

Clever use of drawing out all the keys of LETTER_POOL as a list, here!


def uses_available_letters(word, letter_bank):
pass
""" check each letter used in leter_bank_copy """
letter_bank_copy = letter_bank[:]

Choose a reason for hiding this comment

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

Great use of the python slicing notation to make a copy! ✨

Comment on lines +101 to +110
for word in word_list:
score = score_word(word)
score_tuple = (word, score)
word_and_score.append(score_tuple)

# Create dictionary using word-score pair in each tuple as key and value
word_and_score_dict = {}
for word, score in word_and_score:
word_and_score_dict[word] = score

Choose a reason for hiding this comment

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

Since we're creating a word_and_score list of tuples only to form the word_and_score_dict on L108-110, I'm curious if we could have made this more efficient and tried to make word_and_score_dict from the get-go! 🤔

highest_score = score
best_word = word
# Break tie

Choose a reason for hiding this comment

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

Great tie breaker logic below! I see how you could have combined L121-122 and L127-128 if you wanted, but this reads very clearly!

Comment on lines +64 to +72
while len(letters) < 10:
letter = random.choice(list(LETTER_POOL))
if LETTER_POOL[letter] == 0: # Check letter is in pool
continue
else: # Remove one letter from the letter pool
letters.append(letter)
LETTER_POOL[letter] -= 1

return letters

Choose a reason for hiding this comment

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

One thing to consider: the way this function is structured, this will prevent us from picking a letter that is not available. However, when we randomly pick on L65, it's a random pick from the 26 letters in an alphabet. That means A and Z both have 1/26th chance of being picked before we do the validation check, despite the letter bag distribution for A being 9/98 and Z being 1/98.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants