-
Notifications
You must be signed in to change notification settings - Fork 135
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
Kunzite - Danica S #124
base: main
Are you sure you want to change the base?
Kunzite - Danica S #124
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job! 🎉 Your tests are all passing and your code is well-organized. The main thing I'd encourage is to include comments about your understanding of any library functions being called (or even give a whirl writing them yourself as you did with the code for min
). Sometimes, library functions, while helpful, may conceal hidden costs that we may be able to work around ourselves, as we'll learn more about shortly. Implementing the logic ourselves can help us become aware of that.
Also, take a look at the tie breaking logic in get_highest_word_score
, as it looks like we could end up picking a 10-letter word too quickly.
Overall, well done!
pass | ||
big_list = [] | ||
for letter, letter_quantity in LETTER_POOL.items(): | ||
big_dict = letter * letter_quantity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice use of repetition to handle including multiple cpoies of a letter. Since your objective was to eventually add this to a list, consider
letters = [letter] * letter_quantity
which will make a list rather than a string. This can then be added to your growing list as
big_list.extend(letters)
# or
big_list += letters
big_dict = letter * letter_quantity | ||
for single_char in big_dict: | ||
big_list.append(single_char) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider moving the first part of this function to a helper function that builds an expanded list from a dictionary of keys with counts.
big_list.append(single_char) | ||
|
||
draw_of_10_letters = [] | ||
while len(draw_of_10_letters) < 10: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using a for loop. Since you built an explanded list, you know every pick will be "successful". So we know this will loop exactly 10 times.
|
||
draw_of_10_letters = [] | ||
while len(draw_of_10_letters) < 10: | ||
random_letter = random.choice(big_list) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using a library function like this that we haven't discussed, I encourage you to either include a comment that explains your understanding of how it works, or consider implementing the logic yourself rather than using the library function for additional practice!
while len(draw_of_10_letters) < 10: | ||
random_letter = random.choice(big_list) | ||
draw_of_10_letters.append(random_letter) | ||
big_list.remove(random_letter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we start to discuss big O (coming up) we'll see that removing from a list is realtivelty inefficient. This does guarantee that future picks won't use a letter we've already "consumed", but in a loop, we'd like to avoid remove if possible.
else: | ||
total_score += score_chart[letter] | ||
if 7 <= len(word) <= 10: | ||
total_score += 8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Nice bonus handling.
return ((word, score)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the best_words_list
hasn't been filtered by score (and words can appear in any order), we can't say for certain that the first 10 letter word we find is the winner. There might be a higher scoring 10-letter word later in the list.
If we filtered the list so that the only words in it are those with the highest score, then this assumption would be accurate.
if len(word) == 10: | ||
return ((word, score)) | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The else
here isn't needed. Since the loop will either complete without exiting (triggering the else) or return from the function (in which case, there's no risk of running the code following the loop), we know that if we reach the code after the loop, it can only be due to the look exiting normally (there's no break
).
As such, we can remove the else
and unindent the remaining code.
top_score = 0 | ||
best_word = [] | ||
for word, score in best_words_list: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Nice tuple unpacking during the iteration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice way to get the list of words tied for the best score. Consider doing this logic when building the best_words_list
(which would address the 10-letter word check).
if len(short_word) > len(word): | ||
short_word = word |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job finding the shortest word from among the words tied for the best score.
From the commented code below, it looks like you were also looking into max/min as wyas to accomplish some of this. Now that you've implemented the logic behind min
yourself, I'd definitely suggest incoroporating min/max in future code!
No description provided.