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

Kunzite - Cindy V. #108

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Kunzite - Cindy V. #108

wants to merge 1 commit into from

Conversation

civi10
Copy link

@civi10 civi10 commented Mar 24, 2023

No description provided.

Copy link

@anselrognlie anselrognlie left a 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 focus on is looking more into how some of the library functions you used are implemented, and think about where we might take different approaches to avoid some of their drawbacks. 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. Overall, well done!

def draw_letters():
pass
letter_list = ['A', 'A', 'A', 'A', 'A', 'A', 'A', 'A', 'B', 'B', 'C', 'C', 'D', 'D', 'D', 'D', 'E', 'E', 'E', 'E', 'E', 'E', 'E', 'E', 'E', 'F', 'F', 'G', 'G', 'G', 'H', 'H', 'I', 'I', 'I', 'I', 'I', 'I', 'I', 'J', 'K', 'L', 'L', 'L', 'M', 'M', 'N', 'N', 'N', 'N', 'N', 'N', 'O', 'O', 'O', 'O', 'O', 'O', 'O', 'P', 'P', 'Q', 'R', 'R', 'R', 'R', 'R', 'S', 'S', 'S', 'S', 'T', 'T', 'T', 'T', 'T', 'T', 'U', 'U', 'U', 'U', 'V', 'V', 'W', 'X', 'Y', 'Y', 'Z']

Choose a reason for hiding this comment

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

We do need a copy of the letters each time the function runs since the list is being modified (removing a tiles each call), but instead of hard coding the list, which is error prone due to the repetition (for instance, there are 9 Es in the list, but we're supposed to have 12 of them!), could we make use of the more readable dictionary data to generate the list?



while len(players_hand) < 10:
random_letter = random.choice(letter_list)

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!

word = word.upper()

for letter in word:
if word.count(letter) > letter_bank.count(letter):

Choose a reason for hiding this comment

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

If we were going to implement count ourselves, we would need to iterate through the list. So each time this loops over a letter from word, we are iterating over the word and the bank. Could we do all our counts a different way before we start looping? What tradeoff would we be making to store the results for later use?

random_letter = random.choice(letter_list)
players_hand.append(random_letter)

letter_list.remove(random_letter)

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 realtively 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. To remove a value from a list, we need to iterate to find the value, then continue iterating to the end of the list shifting the remaining values forward (we can't have holes in a list). So each time we add a letter to the hand, we also need to iterate over the letter list. For the length of the letter list in our scenario, this isn't a big deal, but in a more general case, we'd like to avoid this if possible.

Is there some other way we could track which letters we've used without needing to remove them, but also ensuring that we don't pick the same tile again?

def score_word(word):
pass
score_dict = {

Choose a reason for hiding this comment

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

Like the LETTER_POOL, this could live externally to the function (globally) so that it doesn't need to be regenerated with every call to the function.


result = 0
for letter in word.upper():
result += score_dict[letter]

Choose a reason for hiding this comment

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

✅ Great approach to look up the score for each letter and accumulate it into the result.

Comment on lines +92 to +93
if len(word) >= 7:
result += 8

Choose a reason for hiding this comment

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

✅ Nice bonus handling.

score = score_word(word)

if score > high_score:

Choose a reason for hiding this comment

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

✅ Nice job breaking down the conditons into these checks. They read very cleanly.

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