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

C18 Lions Carina Bauman #83

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

Conversation

checarina
Copy link

image

Copy link

@alope107 alope107 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! Really like the helper functions you created here. In the future, please make more frequent and more descriptive commits. Nicely done, this project is green!

Comment on lines +3 to +6
def create_letter_list():
#put the helper function here
LETTER_POOL = {
'A': 9,

Choose a reason for hiding this comment

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

Clever helper!

Comment on lines +33 to +37
letter_pool = []
for letter in LETTER_POOL:
for i in range(LETTER_POOL[letter]):
letter_pool.append(letter)
return letter_pool

Choose a reason for hiding this comment

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

Great logic! One stylistic note: in the inner for loop we never use the i variable. To make it clear that this variable is unneeded we can use an underscore instead: for _ in range(LETTER_POOL[letter]):. This is a common idiom in Python when we want to show we don't care about the value of a variable.

Comment on lines 39 to +46
def draw_letters():
pass
#convert LETTER_POOL dict to list--helper function?
letter_pool = create_letter_list()
hand = []
for i in range(10):
rand_index = random.randint(0, len(letter_pool) - 1)
hand.append(letter_pool.pop(rand_index))
return hand

Choose a reason for hiding this comment

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

Great!

Comment on lines 48 to +58
def uses_available_letters(word, letter_bank):
pass
word = word.upper()
hand_copy = letter_bank[:]
# print(hand_copy)
for letter in word:
if letter not in hand_copy:
hand_copy = letter_bank[:]
return False
hand_copy.remove(letter)
# print(hand_copy)
return True

Choose a reason for hiding this comment

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

This works well! Smart to copy the letter_bank to avoid mutating it. However, line 54 is unneeded.

adagrams/game.py Outdated
Comment on lines 97 to 115
def get_highest_word_score(word_list):
pass No newline at end of file
# print(word_list)
word_scores = []
for word in word_list:
word_scores.append((word, score_word(word)))
high_score = 0
for i in range(len(word_scores)):
if word_scores[i][1] > high_score:
winner = word_scores[i]
high_score = winner[1]
elif word_scores[i][1] == high_score:
if len(winner[0]) == 10:
break
elif len(word_scores[i][0]) < len(winner[0]) or len(word_scores[i][0]) == 10:
winner = word_scores[i]
high_score = winner[1]

return winner

Choose a reason for hiding this comment

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

There's some great logic here, but I think it might not work in some edge cases due to the break on line 110. When we break, we exit the loop entirely and stop considering any new words. In this case, if we find words that are tied in points but one was already 10 letters, we assume that the 10 letter word must be the best of any of our words. This may not be the case. Think through the example below:

Word A: len 10, points 25
Word B: len 10, points 25
Word C: len 10, points 50

I believe your function would incorrectly return Word A, because it would break the loop when checking Word B for a tiebreak and never make it to Word C (the actual highest scoring word).

Also, consider unpacking your variables instead of using the index to make it more readable. For example, we could rewrite line 104 to be for word, score in word_scores:

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.

2 participants