Cohort 22 Adagrams Guevara Alejandra #40
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,102 @@ | ||
| import random | ||
| 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 | ||
| } | ||
|
|
||
| def draw_letters(): | ||
| letter_list = [] | ||
| pool_letter_count = [] | ||
|
There was a problem hiding this comment. The name of this variable is a little confusing for me. With |
||
|
|
||
| for letter, count in LETTER_POOL.items(): | ||
| for _ in range(count): | ||
| pool_letter_count.append(letter) | ||
|
Comment on lines
+35
to
+37
There was a problem hiding this comment. This code does a nice job of building up a list of all the available tiles. We could make this a little clearer by moving the logic to a helper function and giving it a good descriptive name, maybe |
||
|
|
||
| while len(letter_list) < 10: | ||
|
There was a problem hiding this comment. This line effectively has the meaning "run until we've picked 10 tiles." Some approaches to picking a tile aren't guaranteed to pick a tile each time through the loop, so phrasing the loop that way can be necessary. In your case, your loop block will always pick a tile each iteration, and so another way we think of this loop is, run the code 10 times. We could write that as for _ in range(10):similarly to how you did the pile building a little earlier. We could also consider giving a name to |
||
| letter_position = random.randint(0, len(pool_letter_count) -1) | ||
| letter = pool_letter_count[letter_position] | ||
| letter_list.append(letter) | ||
| pool_letter_count.remove(letter) | ||
|
There was a problem hiding this comment. This works to remove one copy of the letter from the list (there could be more than one copy of the letter), but it's not necessarily the position that was picked. This doesn't affect the outcome in this case, but if we know the exact position, we can use pool_letter_count.pop(letter_position)As we start to discuss code complexity (which is about how efficient it is, not how difficult to understand it is), either of these approaches is equivalent. Removing a value from a list by its value or by its position has the same performance. But in this case, since the order of the letters in the pile isn't really important, we could remove the letter a little more efficiently by first swapping the letter to remove to the end of the list, and then popping from the end. last_pos = len(pool_letter_count) - 1
pool_letter_count[last_pos], pool_letter_count[letter_position] = pool_letter_count[letter_position], pool_letter_count[last_pos]
pool_letter_count.pop()Though the code is longer, it's actually more efficient. |
||
|
|
||
| return letter_list | ||
|
|
||
|
|
||
|
|
||
| def uses_available_letters(word, letter_bank): | ||
| letter_bank_list = letter_bank.copy() | ||
|
There was a problem hiding this comment. 👍 This is important so that we don't modify the passed in |
||
| word = word.upper() | ||
|
There was a problem hiding this comment. 👍 This is important because we have tests that show this verification should be case-insensitive. |
||
|
|
||
| for letter in word: | ||
| if letter in letter_bank_list: | ||
| letter_bank_list.remove(letter) | ||
| else: | ||
| return False | ||
|
Comment on lines
+54
to
+57
There was a problem hiding this comment. If I were writing this code, I would reverse the sense of the comparison, like this if letter not in letter_bank_list:
return False
letter_bank_list.remove(letter)The reason I would do this is that it lets us avoid indenting the "important" part of the code. Because of how Python uses indentation to control the flow of the code, we prefer to avoind indentation when we can. There was a problem hiding this comment. The performance of both checking whether something is |
||
| return True | ||
|
|
||
|
|
||
| def score_word(word): | ||
| score_chart = { | ||
| "A": 1, "E": 1, "I": 1, "O": 1, "U": 1, "L":1, "N": 1, "R": 1, "S":1, "T":1, | ||
| "D": 2, "G": 2, | ||
| "B": 3, "C": 3, "M": 3, "P": 3, | ||
| "F": 4, "H": 4, "V": 4, "W": 4, "Y": 4, | ||
| "K":5 , | ||
| "J": 8, "X": 8, | ||
| "Q": 10, "Z": 10, | ||
| } | ||
|
Comment on lines
+62
to
+70
There was a problem hiding this comment. 👍 Nice job storing the scores so that we can look them up quickly using the letter. Consider moving this out of the function (similar to how you have Also, consider ordering the letters alphabetically. The layout here is very close to how it was shown in the README, but as a person, if I wanted to quickly find one of the letters (maybe I need to update a score), it would be helpful if the code was arranged alphabetically. It would also help us double check that we've got all the letters accounted for. Keep in mind that the primary consumer of code is not the computer, it's other coders. So laying out the data and logic in a way that's easy for others to follow is important. |
||
| score = 0 | ||
| word = word.upper() | ||
|
|
||
| for letter in word: | ||
| letter_point_value = score_chart[letter] | ||
| score += letter_point_value | ||
|
Comment on lines
+74
to
+76
There was a problem hiding this comment. 👍 Great job calculating the base word score by summing the scores of the individual letters. |
||
| if len(word) >= 7: | ||
| score += 8 | ||
|
Comment on lines
+77
to
+78
There was a problem hiding this comment. 👍 Nice handling of adding the bonus. We can also write this to make it more clear why we're doing this. Consider giving names to the "magic numbers" (hard coded literal values that appear in code) that are used here. We could use |
||
| return score | ||
|
|
||
|
|
||
| def get_highest_word_score(word_list): | ||
| highest_score = 0 | ||
| highest_score_word = "" | ||
|
|
||
| for word in word_list: | ||
| current_score = score_word(word) | ||
| if current_score > highest_score: | ||
|
There was a problem hiding this comment. 👍 A higher score word always replaces the best word so far. |
||
| highest_score = current_score | ||
| highest_score_word = word | ||
| elif current_score == highest_score: | ||
| if len(highest_score_word) == len(word) and len(highest_score_word) == 10: | ||
| continue | ||
| elif len(word) < len(highest_score_word) and len(highest_score_word) < 10: | ||
| highest_score_word = word | ||
| elif len(highest_score_word) < 10 and len(word) == 10: | ||
| highest_score_word = word | ||
|
Comment on lines
+92
to
+97
There was a problem hiding this comment. This first condition isn't necessary for the logic. Conditions like that can be helpful while initially planning our logic, but if we can remove them later, it's a good idea. One way to avoid bugs is to have less code for there to be bugs in. To be clear, there's no bug in the submitted code, but any other coder that comes along and reads the code will have to try to understand why it's here. We can save them that effort by removing it. if len(highest_score_word) < 10 and len(word) == 10:
highest_score_word = word
elif len(word) < len(highest_score_word) and len(highest_score_word) < 10:
highest_score_word = word Be sure to take some tie to think about why removing that condition was safe to do. It would even be possible to combine both of the checks into a single check (combined with Another different approach is to break up the parts of the logic into different chunks rather than trying to do it all in one loop. We can think of another approach like:
The tie-breaking logic becomes a little easier thinking of the problem this way! |
||
|
|
||
| return highest_score_word, highest_score | ||
|
|
||
|
|
||
|
|
||
|
There was a problem hiding this comment. Try to avoid introducing unintentional changes to files. If we see a change show up in a file we didn't intend, we can revert a change so that it doesn't get included in a commit. git restore tests/test_wave_02.py |
|
There was a problem hiding this comment. Try to avoid introducing unintentional changes to files. If we see a change show up in a file we didn't intend, we can revert a change so that it doesn't get included in a commit. git restore tests/test_wave_04.py |
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.
It looks like there was only a single commit for the whole project. Try to commit more frequently on future projects. After finishing up each wave is a great time to commit your work so far.