C22/Natalie Sen/Adagrams Wave 1-4 #23
Conversation
| def draw_letters(): | ||
| pass | ||
| # define the dictionary with the letters and their value | ||
| LETTER_POOL = { |
There was a problem hiding this comment.
You are currently using the naming convention for a Global variable on LETTER_POOL (All caps) but placing it as a local variable. I would agree that this is should be a global variable, in which case make sure to move it outside the function!
| def pick_random_letter(): | ||
| # choose a letter randomly from the dict | ||
| letters = [letter for letter, value in LETTER_POOL.items() if value > 0] # looping through LETTER_POOL to get the value of the letters only if the value is bigger than 0 | ||
| if not letters: | ||
| return None #if there are no available letters, return None | ||
| # use random.randint to pick a letter by index | ||
| index = random.randint(0, len(letters) - 1) # generates a random index between 0 and len(letters) - 1 | ||
| return letters[index] | ||
|
|
||
| draw_letters = [] | ||
|
|
||
| for i in range(10): #select exactly 10 letters from dict | ||
| letter = pick_random_letter() | ||
| if letter is None: # if None is returned indicating no more letters available, break the loop | ||
| break | ||
| draw_letters.append(letter) # adding the randomly picked letters into the initial empty list | ||
| LETTER_POOL[letter] -= 1 # decrease the count of the drawn letter in the dict | ||
| if LETTER_POOL[letter] == 0: # if the count of a letter reaches 0, removes the letter from the dict | ||
| del LETTER_POOL[letter] # remove the letter if its value reaches 0 | ||
|
|
||
| return draw_letters |
There was a problem hiding this comment.
There is some really cool stuff in this function/algorithm that you wrote. The nested function is well written and can be useful in a variety of situations. Typically however, we'll want to use a nested function only if it makes sense and I don't think this is one of those situations. Your pick_random_letter() function is technically doing what you want it, but you are making certain checks that we don't actually need to make! In a sense, you are overcomplicating your code here.
One place where I think there's a bit of confusion is how you are handling LETTER_POOL. It is true that you are destroying LETTER_POOL as you remove the tiles that get added to your hand, so you might potentially need to make checks to see if you have enough tiles left. My concern with this particular nested function is that it recreates the list comprehension on line 36 every time it gets called. It would be much better if we had a more direct way of figuring out our range for randint.
I think that overall, the best way to refactor this would be to use LETTER_POOL to make a list of every single potential letter. From there, you can run your while loop. Within the while loop, use randint to select single letters from the newly created list. Because that list is essentially a copy, we can remove the letters and essentially destroy the list without worrying about needing to use it again later.
| pass | ||
| # function that counts each letters that occured in the list of letters | ||
| # this part of the code can be replaced by import counter | ||
| def count_letters(letters): |
There was a problem hiding this comment.
I love the idea of a helper function here to make dictionaries out of a word count! My one concern is that it is currently a nested function. Typically we only nest functions if they only apply to the function they are being defined in. This is one that could actually be pretty useful elsewhere or in general, so I would make it it's own function rather than nesting it!
|
|
||
| # convert word and letter_bank to lowercase to handle case sensitiveness | ||
| word = word.lower() | ||
| letter_bank = [letter.lower() for letter in letter_bank] |
| for letter, count in word_counts.items(): | ||
| if letter_bank_counts.get(letter, 0) < count: | ||
| return False # if the letter count in word is more than what we have in letter_bank | ||
| return True # if all the letters are available in letter_bank |
There was a problem hiding this comment.
The rest of this function is super clean! Well done!
|
|
||
| def score_word(word): | ||
| pass | ||
| # score map, value of each letters | ||
| letter_scores = { |
There was a problem hiding this comment.
This dictionary looks good! Just a heads up that it could also be seen as a global variable that could be used elsewhere!
| base_score += letter_scores.get(letter, 0) | ||
|
|
||
| # calculate the bonus points for the words od length 7 or more | ||
| if len(word) >= 7: |
There was a problem hiding this comment.
Don't forget that we don't add 8 if the word goes over 10 letters! That's a similar condition!
| else: | ||
| bonus_points = 0 | ||
|
|
||
| total_score = base_score + bonus_points |
There was a problem hiding this comment.
As you get more comfortable with python, I would encourage you to keep the score to a single variable for the entirety of the function! It helps streamline things just a bit! This would replace the instinct to separate out the score into base and bonus and just keep it all in one place and add it up as the function progresses.
| highest_word = word | ||
| # assign current word to highest word if the lengths of the current word and highest word are the same | ||
| elif len(word) == len(highest_word) and highest_word == "": |
There was a problem hiding this comment.
What the comment says this if statement is doing is very different than what the if statement is actually doing! You've included a check to see if highest word doesn't actually exist. The only way this would ever be triggered is if the list is just empty strings, which we know isn't the case, so this final elif is actually redundant. Other than that, the logic you have here is great!
| highest_word = word | ||
|
|
||
| return (highest_word, highest_score) |
There was a problem hiding this comment.
Just a small thing here, but we don't actually need the parentheses. If we just return two values, the return statement will tupelize (definitely not a word, but I couldn't resist) the returns by default!
No description provided.