Cohort 22 Adagrams Guevara Alejandra #40
Conversation
There was a problem hiding this comment.
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.
| def draw_letters(): | ||
| pass | ||
| letter_list = [] | ||
| pool_letter_count = [] |
There was a problem hiding this comment.
The name of this variable is a little confusing for me. With count in the name, it sounds like there should be numbers stored in the list rather than the letters. Maybe a name like all_letter_tiles would help the reader underdstand.
| for letter, count in LETTER_POOL.items(): | ||
| for _ in range(count): | ||
| pool_letter_count.append(letter) |
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 build_tile_pile.
| for _ in range(count): | ||
| pool_letter_count.append(letter) | ||
|
|
||
| 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 10, like HAND_SIZE so that it's clear what this number represents here. For a small project like this, we probably know what all the numbers represent, but for larger systems, having unnamed numbers showing up all over the place makes the code more difficult to understand.
| 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 pop rather than remove.
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.
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| for letter in word: | ||
| letter_point_value = score_chart[letter] | ||
| score += letter_point_value |
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 |
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 MIN_BONUS_LEN for 7, and LENGTH_BONUS for 8. We could even move this to a helper function called something like add_bonus_points.
| 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.
| 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 |
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 or), but that would make the condition much more difficult to understand. But there are potentially other ways to combine the checks that might still be understandable, so try to think about that a bit.
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:
- Find the max score (any winning word must have this score)
- Make a list of all the words that have the max score
- If we found only a single word, that's the winner
- Otherwise, look at each word
- If we find a 10 letter word, that's the winner (nothing would be able to beat it)
- Otherwise, find the shortest word, that's the winner
The tie-breaking logic becomes a little easier thinking of the problem this way!
Cohort 22 - Phoenix - Alejandra Guevara