Skip to content

Whales - Alexa Coffman and Uyen Vong#36

Open
AlexaCoffman wants to merge 6 commits intoada-c17:masterfrom
AlexaCoffman:master
Open

Whales - Alexa Coffman and Uyen Vong#36
AlexaCoffman wants to merge 6 commits intoada-c17:masterfrom
AlexaCoffman:master

Conversation

@AlexaCoffman
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown

@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.

Wonderful work, Alexa and Uyen! 🚀

Your tests pass, your logic is superb, and I can wax poetic about your spiffy update_best_word function all day long. Y'all have made great choices regarding your data types and your code is exceptionally clean and efficient. This project is a Green. 🟢

Let me know if you have any questions on the feedback. Keep it up!

Comment thread adagrams/game.py
Comment on lines +42 to +44
while len(drawn_letters) < 10:
rand_index = randint(0, len(letter_pool_copy)-1)
drawn_letters.append(letter_pool_copy.pop(rand_index))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Interesting way to randomly pick a letter! I also love that you modeled your LETTER_POOL list to contain the correct frequency of letters in a bag. Great work!

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

But also...what if the instructors were feeling ~ chaotic ~ and said there were 100,000 tiles in the pool in total? Could you have programmatically made the LETTER_POOL list? (I'm sure you could have!)

Comment thread adagrams/game.py
# create a copy of letter bank so we don't alter original list
letter_bank_copy = deepcopy(letter_bank)
word = word.upper()
valid_word = True
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is soooo pythonic! 🐍 I adore how y'all set a boolean for valid_word at the top of the function and reassign on L56 depending on the condition, then return this boolean only once in L60! While the following is acceptable:

if [valid conditional]:
    return True
else:
    return False

What y'all did (set a boolean variable to have only one return statement at the end of the function, like below) is a Pythonista's dream! Great work!

valid = True
if [not valid conditional]:
    valid = False
return valid

Comment thread adagrams/game.py
Comment on lines +77 to +78
if word == None:
return None
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Great validation check!

Comment thread adagrams/game.py
Comment on lines +89 to +92
best_word = {
"word" : "",
"score" : 0
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Love the organization of best word and score as a dictionary! 🤩

Comment thread adagrams/game.py
Comment on lines +107 to +109
def update_best_word(best_word, word, score):
best_word["score"] = score
best_word["word"] = word
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Lovely helper function! 💯

Comment thread adagrams/game.py
def update_best_word(best_word, word, score):
best_word["score"] = score
best_word["word"] = word
return best_word No newline at end of file
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Python (and most languages, if I think of it) likes it when you have a newline at the end of the file. (Why? Because it's one of those Just Unix/Computer Things, I guess?)

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