Cohort_22_Phoenix_Marjana_Khatun#47
Conversation
kelsey-steven-ada
left a comment
kelsey-steven-ada
left a comment
There was a problem hiding this comment.
Great work Marjana! Let me know if you have questions on any of the feedback =]
| } | ||
|
|
||
|
|
||
| list_letters = [] |
There was a problem hiding this comment.
This is a duplicate of the variable we declare on line 4, so one of them should be removed.
| def draw_letters(): | ||
| pass | ||
| list_letters = [] | ||
| distribution_of_letters = { |
There was a problem hiding this comment.
Nice placement of the distribution_of_letters structure in the function. It's a tradeoff of visually taking up space in the function so we don't need to worry about changes to the structure between function calls.
| while len(list_letters) != 10: | ||
| letters = list(distribution_of_letters.keys()) | ||
| random_letter = random.choice(letters) | ||
| value = distribution_of_letters[random_letter] | ||
| if value > 0: | ||
| distribution_of_letters[random_letter] = value -1 | ||
| list_letters.append(random_letter) |
There was a problem hiding this comment.
There is a subtle issue with how we're selecting tiles. We are guaranteed to always get a hand of 10, and we are picking from the full alphabet and ensuring we aren't picking too many of a certain tile however, we aren't taking into account the correct probability of grabbing a particular tile.
When we grab a tile, we are picking from the keys of distribution_of_letters. This means we have an equal 1 in 26 chance to grab any letter. However, based on our distributions, we should have many more of some tiles than others, which means we should be more likely to pick those tiles than others.
We can get around this a number of ways; one possibility is creating a list containing all of our possible tiles, then remove each tile from this list as we randomly select them.
| list_letters = [] | ||
| while len(list_letters) != 10: | ||
| letters = list(distribution_of_letters.keys()) | ||
| random_letter = random.choice(letters) |
There was a problem hiding this comment.
In this cohort we shared guidance during Unit 1 asking folks to stick to using randint and avoid functions from the random package that do more of the random selection work like sample and choice. How would your implementation change using only random.randint?
| else: | ||
| return False |
There was a problem hiding this comment.
Nice early exit as soon as we find a letter that isn't in the letter_bank!
| for letter in word: | ||
| if letter in letters_copy: | ||
| letters_copy.remove(letter) |
There was a problem hiding this comment.
This is a clear and concise solution, but what is the time complexity like? For each letter in word we may need to make a full loop over letters_copy. If letter exists in letters_copy, the remove method is O(n) in the worst case where we remove the first element and need to shift all elements down.
How could we use frequency maps to reduce our time complexity?
| total_score = 0 | ||
| word = word.upper() | ||
| for letter in word: | ||
| value = score.get(letter, 0) |
There was a problem hiding this comment.
Great use of get to handle non-letter characters like hyphens!
| "Z" :10 | ||
| } | ||
| total_score = 0 | ||
| word = word.upper() |
There was a problem hiding this comment.
In this case it isn't a big deal, but we typically don't want to overwrite our parameter variables since we lose access to the original value.
| elif len(word)== 10: | ||
| highest_word = word | ||
| elif len(word)<len(highest_word): | ||
| highest_word = word | ||
| return(highest_word,highest_score) |
There was a problem hiding this comment.
Reminder about spacing around operators and commas to make statements easier to read quickly.
| elif len(word)== 10: | |
| highest_word = word | |
| elif len(word)<len(highest_word): | |
| highest_word = word | |
| return(highest_word,highest_score) | |
| elif len(word) == 10: | |
| highest_word = word | |
| elif len(word) < len(highest_word): | |
| highest_word = word | |
| return(highest_word, highest_score) |
| if len(highest_word) == 10: | ||
| continue | ||
| elif len(word)== 10: | ||
| highest_word = word | ||
| elif len(word)<len(highest_word): | ||
| highest_word = word |
There was a problem hiding this comment.
We don't take an action inside the if statement and both sides of the elif blocks have the same contents highest_word = word. Repeating code like this is often a sign that we should look at our code to see if there are ways we could combine our statements to reduce repetition.
Thinking about long-term code maintenance, if we have to change that code in the future, it goes quicker and is less error-prone when there is only one line to update, rather than several lines that we need to keep in sync.
One of many options could be to create variables for the boolean statements:
is_new_word_ten = len(word) == 10 and len(highest_word) != 10
is_new_word_smaller = len(word) < len(highest_word) and len(highest_word) != 10
if is_new_word_ten or is_new_word_smaller:
highest_word = word2 participants
No description provided.