Phoenix - Ajar Duishembieva #26
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,156 @@ | ||
| import string | ||
| import random | ||
|
|
||
| def draw_letters(): | ||
| LETTER_POOL = { | ||
| 'A': 9, | ||
| 'B': 2, | ||
| 'C': 2, | ||
| 'D': 4, | ||
| 'E': 12, | ||
| 'F': 2, | ||
| 'G': 3, | ||
| 'H': 2, | ||
| 'I': 9, | ||
| 'J': 1, | ||
| 'K': 1, | ||
| 'L': 4, | ||
| 'M': 2, | ||
| 'N': 6, | ||
| 'O': 8, | ||
| 'P': 2, | ||
| 'Q': 1, | ||
| 'R': 6, | ||
| 'S': 4, | ||
| 'T': 6, | ||
| 'U': 4, | ||
| 'V': 2, | ||
| 'W': 2, | ||
| 'X': 1, | ||
| 'Y': 2, | ||
| 'Z': 1 | ||
| } | ||
| my_dictionary = {} | ||
|
There was a problem hiding this comment. I see that you have two collections to hold the hand of cards that gets picked. As far as the rest of this function goes, you then store what gets picked in the dictionary and then move everything to the list to be returned. With this in mind, is the dictionary actually necessary? What if we just placed everything in the list to begin with? |
||
| my_list = [] | ||
| while len(my_dictionary) != 10: | ||
|
There was a problem hiding this comment. To go along with my previous comment, it's important to remember how length for a dictionary works. We have multiples of many letters in our dictionary. Later on, you account for this by making sure the count of a specific letter in your hand doesn't go past the count of the letter in |
||
| letter = random.choice(string.ascii_uppercase) | ||
|
There was a problem hiding this comment. I like the way you found a way to get a string of all uppercase letters here. I wonder if there is a way to approach this randomness without using the |
||
| if letter in my_dictionary: | ||
| value = my_dictionary[letter] | ||
| if value < LETTER_POOL[letter]: | ||
| my_dictionary[letter] = value + 1 | ||
| else: | ||
| my_dictionary[letter] = 1 | ||
|
|
||
| for key in my_dictionary: | ||
|
There was a problem hiding this comment. With this for loop, you are only looping through the keys and adding them to the list. This unfortunately doesn't account for multiples of a letter that may have been selected. |
||
| my_list.append(key) | ||
| return my_list | ||
|
|
||
| def uses_available_letters(word, letter_bank): | ||
| word_dictionary = {} | ||
| for letter in word.upper(): | ||
| if letter in word_dictionary: | ||
| value = word_dictionary[letter] | ||
| word_dictionary[letter] = value + 1 | ||
| else: | ||
| word_dictionary[letter] = 1 | ||
|
|
||
| for letter in letter_bank: | ||
| if letter in word_dictionary: | ||
| value = word_dictionary[letter] | ||
| word_dictionary[letter] = value - 1 | ||
| return sum(word_dictionary.values()) == 0 | ||
|
There was a problem hiding this comment. I love this return statement! Super clever! As far as the rest of the function goes, it looks good and works the way we're expecting! One extension possibility: Is it possible to do this without an extra dictionary? If we made a copy of our letter_bank, how could we condense this code just a bit? |
||
|
|
||
| def score_word(word): | ||
| LETTER_SCORES = { | ||
|
There was a problem hiding this comment. Same idea as before! This is a great global variable, but as such, it might be better served above all the functions! This is a common practice when we are working with global variables, even if they don't apply to more than one function. |
||
| 'A': 1, | ||
| 'B': 3, | ||
| 'C': 3, | ||
| 'D': 2, | ||
| 'E': 1, | ||
| 'F': 4, | ||
| 'G': 2, | ||
| 'H': 4, | ||
| 'I': 1, | ||
| 'J': 8, | ||
| 'K': 5, | ||
| 'L': 1, | ||
| 'M': 3, | ||
| 'N': 1, | ||
| 'O': 1, | ||
| 'P': 3, | ||
| 'Q': 10, | ||
| 'R': 1, | ||
| 'S': 1, | ||
| 'T': 1, | ||
| 'U': 1, | ||
| 'V': 4, | ||
| 'W': 4, | ||
| 'X': 8, | ||
| 'Y': 4, | ||
| 'Z': 10 | ||
| } | ||
| total_points = 0 | ||
| for letter in word.upper(): | ||
| if letter in LETTER_SCORES: | ||
| value = LETTER_SCORES[letter] | ||
| total_points += value | ||
|
There was a problem hiding this comment. This isn't actually an issue, because it is super common practice to see what is happening at each step, but as you get more comfortable with code, I encourage combining this line of code with the one above it! This just allows us to complete two steps with a single line: |
||
| if len(word) == 7 or len(word) == 8 or len(word) == 9 or len(word) == 10: | ||
|
There was a problem hiding this comment. This looks good, but it does start to get a little wordy! Is there a way we could achieve the same result with a compound conditional? |
||
| total_points = total_points + 8 | ||
| return total_points | ||
|
|
||
| def get_highest_word_score(word_list): | ||
| LETTER_SCORES = { | ||
|
There was a problem hiding this comment. As mentioned before, if we're going to use this same dictionary as before, throw it outside the functions! |
||
| 'A': 1, | ||
| 'B': 3, | ||
| 'C': 3, | ||
| 'D': 2, | ||
| 'E': 1, | ||
| 'F': 4, | ||
| 'G': 2, | ||
| 'H': 4, | ||
| 'I': 1, | ||
| 'J': 8, | ||
| 'K': 5, | ||
| 'L': 1, | ||
| 'M': 3, | ||
| 'N': 1, | ||
| 'O': 1, | ||
| 'P': 3, | ||
| 'Q': 10, | ||
| 'R': 1, | ||
| 'S': 1, | ||
| 'T': 1, | ||
| 'U': 1, | ||
| 'V': 4, | ||
| 'W': 4, | ||
| 'X': 8, | ||
| 'Y': 4, | ||
| 'Z': 10 | ||
| } | ||
|
|
||
| word_result = ['0',0] | ||
| for current_word in word_list: | ||
| current_sum = 0 | ||
| for letter in current_word: | ||
| key = letter.upper() | ||
| current_sum = current_sum + LETTER_SCORES[key] | ||
|
|
||
| if len(current_word) == 10: | ||
| current_sum = current_sum + 8 | ||
|
Comment on lines
+134
to
+139
There was a problem hiding this comment. This is code we've already seen in a different function... Is there a way for us to leverage the code that we have already written to achieve the same result? |
||
|
|
||
| previous_sum = word_result[1] | ||
| previous_word = word_result[0] | ||
|
|
||
| if previous_sum < current_sum: | ||
| word_result[0] = current_word | ||
| word_result[1] = current_sum | ||
| elif previous_sum == current_sum: | ||
| previous_size = len(previous_word) | ||
| current_size = len(current_word) | ||
|
|
||
| if(current_size==10 and previous_size !=10): | ||
| word_result[0] = current_word | ||
| elif current_size!=10 and previous_size !=10 and current_size < previous_size: | ||
| word_result[0] = current_word | ||
|
Comment on lines
+141
to
+154
There was a problem hiding this comment. The logic here looks great! The main area for improvement I see is that you add new variables at each level of nesting. There is nothing inherently wrong with that, but it can start to get difficult to read and understand. In the future, see if there are ways you can reuse variables you've already declared! |
||
|
|
||
| return tuple(word_result) | ||
|
There was a problem hiding this comment. It is possible to return a tuple without casting it from another data type! If we have two variables, we can return them both in one statement. For example, if I had two variables called to return them as a tuple. |
||
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.
This is a great dictionary to hold the letters and the count of each letter! When creating it, you've followed the naming convention of a global variable which I agree would be the best way to hold this information just in case we need to use this dictionary elsewhere! The one change I would make here would be to move it outside this function which is what we would do to make sure the dictionary is available to the other functions that might need it. If we have several pieces of this kind of information, it is also possible to put your data in a separate file and import it in as well!