Phoenix -- Beenish Ali #30
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.
| @@ -1,11 +1,134 @@ | |||
| import random | |||
| import sys | |||
There was a problem hiding this comment.
It looks like you imported sys to get the maxsize. That works, and since you're not grabbing a system helper function, doesn't violate the spirit of what we asked around imports, but there's an approach for locating something that's "the shortest" which doesn't require this. Check my feedback down in get_highest_word_score for more on this.
| 'Z': 1 | ||
| } | ||
|
|
||
| SCORE_CHART = { |
There was a problem hiding this comment.
👍 Nice decision to make the score table another global CONSTANT. And using a dictionary gives us a very efficient way to look up the scores. I also like that you listed the letters alphabetically, as that helps a reader quickly understand that all of the letters do properly have a score, and it's easy to find a specific letter if we need to make any changes.
| letters = list(LETTER_POOL.keys()) | ||
| used_letters = {} | ||
| while len(drawn_letters) < 10: | ||
| random_letter_pos = random.randint(0,25) |
There was a problem hiding this comment.
Try to give names to any numbers appearing here so that we know what they represent, or calculate them from some other relevant piece of data. For example, we can calculate the top of the randint from the length of letters. This would make it easier to maintain our code if we needed to work with different numbers of tiles (maybe we need characters from another language, or we allow certain punctuation). By tying the random range to the list we'll be using the index in, that's one less bit of data that could get out of sync if we make changes to our code.
A subtle point to notice also is that with this approach (picking a letter, and then seeing whether we have enough to use), has a slightly different probability of picking various letters. You'll never pick more than the available letters (so you'll never have more than one Z, for example), but your chances of getting a Z at all are higher with this approach. For example, picking the first letter, this approach has a 1 in 26 chance of drawing a Z, but if we were to consider the letters as all being in a big pile of tiles, we'd only have a 1 in 98 (total number of tiles) change of getting a Z. Effectively, this means we have a much higher chance of drawing "difficult" letters with this approach.
| letter_count = LETTER_POOL[random_letter] | ||
| if random_letter in used_letters: | ||
| used_count = used_letters[random_letter] | ||
| if used_count < letter_count: |
There was a problem hiding this comment.
Notice that because we have to check the number of the picked letter used so far, we aren't guaranteed to successfully pick a letter each time through the loop. Though unlikely, it's possible we could get "stuck" picking letters we've used up already, never finishing picking a hand (or at least taking longer than expected). Even when there's a random process involved, we like to write logic that is guaranteed to make progress, rather than logic that could behave unpredictably.
Think about some approaches that we could use to ensure that we definitely pick a letter each time we iterate.
| for possible_winner in possible_winners: | ||
| if len(possible_winner[0]) == 10: | ||
| return which_came_first(word_list, 10, possible_winners) |
There was a problem hiding this comment.
In fact, since all of the possible_winners are tied for the max score, as soon as we find a 10 letter word, we can return directly, without the which_came_first helper. This is because, while we don't usually think of a dictionary as being ordered, they do preserve the order the keys were inserted. So as we're iterating over the keys to extract the possibel winners, the original word order is preserved!
return possible_winner| return which_came_first(word_list, 10, possible_winners) | ||
| if len(possible_winner[0]) < min: | ||
| min = len(possible_winner[0]) |
There was a problem hiding this comment.
If we also kept track of which word had the new min, then when we got to the end, w would be able to return that word directly. As above, the word order is preserved, even though we went through a dictionary (which we often think of as unordered) in the middle. Something like
min = len(possible_winner[0])
min_word = possible_winnerthen later
return min_word| score += 8 | ||
| return score | ||
|
|
||
| def which_came_first(original_list, num, possible_winners): |
There was a problem hiding this comment.
Nice helper function to enforce the ordering of the original word list. Interestingly, even though this may seem necessary, since we used a dictionary in the middle of processing the words, it turns out to not be the case. Even though dictionaries don't store their keys ordered relative to the keys themselves, they do maintain the order they are inserted into the dictionary, meaning that all of the operations that were done by iterating over the words, even after using the dictionary, still preserve the original word order. With the suggested changes below, this helper could be removed entirely.
| if len(possible_winner[0]) < min: | ||
| min = len(possible_winner[0]) | ||
| return(which_came_first(word_list, min, possible_winners)) |
There was a problem hiding this comment.
Nit: return isn't a function, so don't include parentheses unless we need to wrap a line. Even then, leave a space between return and the parens so that it looks less like a function call.
| drawn_letters = [] | ||
| letters = list(LETTER_POOL.keys()) | ||
| used_letters = {} | ||
| while len(drawn_letters) < 10: |
There was a problem hiding this comment.
Since each loop might not result in adding an additional letter to the hand, we don't know exactly how many times the loop will run. Running until our hand has the desired number of tiles will make sure the loop runs as many times as necessary.
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.
No description provided.