Conversation
Implemented function uses_available_letters
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Looks good! Please review my comments, and let me know if you have any questions. Slack, our next 1:1, or office hours are preferred (rather than replying to my comments in GitHub) as I don't have email notifications turned on. Nice job! 🎉
There was a problem hiding this comment.
🎉 Nice job doing commits regularly as you finished each wave. A commit after each wave is a good target to aim for with projects. When working on your own code, it will be up to you to decide when it's a good time to commit, but the more we practice now, the more we'll be in the habit of committing. Otherwise, it's easy to forget.
| count = 0 | ||
|
|
||
| for letter in LETTER_POOL: | ||
| for value in range(LETTER_POOL[letter]): |
There was a problem hiding this comment.
Notice that we don't use the variable value in the loop body. Python does require that we provide a loop variable name here, but if we're not intending to actually use it, a common variable name is just the underscore character _. This tells the reader of our code that the only reason that variable is there is because Python requires it, not because we're doing anything with it.
This is most common in for _ in range(some_value): loops, where we just need to run the code in the block some_value times, without needing to know which iteration we're on while doing it.
| for letter in LETTER_POOL: | ||
| for value in range(LETTER_POOL[letter]): | ||
| letters_list.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_letter_list.
| letters_list.append(letter) | ||
|
|
||
|
|
||
| while count < 10: |
There was a problem hiding this comment.
To loop 10 times, it's recommended to use a for loop, like
for _ in range(10):since there's no danger of forgetting to initialize count, or updating count in the loop.
Alternatively, we can phrase this as
while len(letters_in_hand) < 10:which links the number of iterations to the work we actually need to do in the loop. We need to be building up the list of letters in our hand as we're looping (that's the whole point), so tying the loop condition to the work we need to do helps the reader focus on the important part of the loop.
| while count < 10: | ||
| random_letter_index = random.randint(0, len(letters_list) - 1) | ||
| letters_in_hand.append(letters_list[random_letter_index]) | ||
| letters_list.pop(random_letter_index) |
There was a problem hiding this comment.
👍 Using pop (rather than remove) lets us extract precisely the letter that was randomly picked. However, we'll see that popping from an aribtrary location has the same performance as using remove (it is related to the length of the list). But if we know exactly which position we want to pop, and the order of things in the list doesn't really matter, we can use a trick to pop from an aribrary location in a way that doesn't depend on the length of the list. We can swap the value we want to pop to the end of the list, then pop from the end (popping from the end doesn't depend on the length of the list). Consider something like
last_pos = len(letters_list) - 1
letters_list[last_pos], letters_list[random_letter_index] = letters_list[random_letter_index], letters_list[last_pos]
letters_list.pop()| if len(word_upper) >= 7 and len(word_upper) <= 10: | ||
| score += 8 |
There was a problem hiding this comment.
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 (may not even be necessary to check the upper length), and LENGTH_BONUS for 8. We could even move this to a helper function called something like add_bonus_points.
| for letter in word_upper: | ||
| score += score_chart_dict[letter] |
There was a problem hiding this comment.
👍 Great job calculating the base word score by summing the scores of the individual letters.
| win_word_list.clear() | ||
| win_word_list.append(word) |
There was a problem hiding this comment.
Alternatively
win_word_list = [word]| for win_word in win_word_list: | ||
| win_word_length = len(win_word) | ||
| if win_word_length < min_length: | ||
| min_length = win_word_length | ||
| win_word_with_min_length = win_word |
There was a problem hiding this comment.
👍 Nice approach to find the shortest word, which among the words tied for the highest score, as long as there are no 10 letter words (checked earlier) the shortest word is the winner.
| return score | ||
|
|
||
|
|
||
| def get_highest_word_score(word_list): |
There was a problem hiding this comment.
Great job of breaking this process down into a series of steps that leads us to finding the winning word. While it's possible to do this "all at once" in a single loop, the "time complexity" (we'll start talking about this for Big O soon) is identical to what we have here, even though there appears to be more looping.
Because you've broken things down into a sequence of distinct steps, we could make this even more self-documenting by moving each step into a helper function with a descriptive name. Consider find_max_score, find_tied_winning_words, find_first_ten_letter_word, etc...
Then reading the function calls would more or less be describing the overall logic of the function rather than needing to go line-by-line, section-by-section, reminding ourselves, "oh yeah, this part is finding the list of words tied for the winning score".
2 participants
No description provided.