Zoisite - Gweneth #117
Zoisite - Gweneth #117
Conversation
There was a problem hiding this comment.
Nice job on adagrams, Gwen!
All your tests are passing and your code is laid out well overall. Nice use of using some built-in functions as well your own approaches.
The logic for creating a distribution of tiles to draw from when creating a hand of 10 is a little off so have a look at the comment I left there. I left a couple of minor suggestions about naming variables too.
There are a couple of places that I suggested could be potential refactors to make your code more readable. Having said that, you met your learning goals and it seems you have a good handle on iterating over data structures!
I can see you made several commits while working. Just a nudge to use descriptive commit messages so that others (and your future self) can quickly glance at the commit history and see what was done at each commit.
Great work!
| tiles = [] #list that's ten letters | ||
| count = 0 #going to stop at 10 |
There was a problem hiding this comment.
Consider renaming the variable tiles to something like list_of_ten_tiles and then you won't need to add a comment to explain what the variable is for.
If you rename count to something like length_of_hand then when I read the while loop on 36, I can understand that the number of tiles in a hand shouldn't exceed ten. Count is a little vague here, renaming the variable would enable you to get rid of the comment too on line 34 too.
| tiles = [] #list that's ten letters | ||
| count = 0 #going to stop at 10 | ||
| letters_dict = LETTER_POOL.copy() | ||
| while count != 10: |
There was a problem hiding this comment.
What if we accidentally introduced a bug and somehow count became 11? Would the loop still run?
Could we make the condition here a little more specific? Like while count < 10:
| count = 0 #going to stop at 10 | ||
| letters_dict = LETTER_POOL.copy() | ||
| while count != 10: | ||
| letter = random.choice(list(letters_dict.keys())) |
There was a problem hiding this comment.
list(letters_dict.keys()) returns a list with 26 elements which are the letters A-Z. This doesn't quite represent the distribution of letters we should be selecting a random tile from. Instead, random.choice() should be passed in a list that has 9 As, 2 Bs, 3 Cs, etc.
How would you create a list of letters that represents all the available tiles to draw from by using letters_dict? Could you iterate through the key value pairs of the dictionary and append to a list like all_available_tiles to create this pool to draw from?
| else: | ||
| continue |
There was a problem hiding this comment.
Do we need this else statement here since we only ask it to continue?
On line 38, if a the value of a letter is greater than 0, then we do some logic on lines 39-41. If if letters_dict[letter] > 0: is false, then it won't enter that block of code and nothing will execute and the loop starts over again so we can drop lines 42-43 altogether.
| pass | ||
| letter_bank_dict = {} | ||
| word_dict = {} | ||
| word = word.upper() |
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Delete extra white space to keep your file neat and concise
|
|
||
|
|
||
| draw_letters() |
There was a problem hiding this comment.
Looks like you added a function call to the end of this file. Be sure to delete unnecessary function calls when you're done working on your project.
While not a big deal here and the tests still pass, imagine a scenario where this method actually calls a database to get the letters for the hand. That call to the database to get the data may take a long time so accidentally leaving in a function call like this could increase latency time for production users so we should be careful about this.
| for word in word_list: | ||
| points = score_word(word) | ||
| if points not in words_points.keys(): |
There was a problem hiding this comment.
If you only need to iterate over the keys, we don't need .keys(). We can find if a key is in a dict just with if key not in dict
| words_points[points].append(word) | ||
|
|
||
| highest_key = max(words_points.keys()) |
There was a problem hiding this comment.
Same comment as above with .keys(), we can find the max value of the keys in a dict just with max(words_points)
It is nice to be able to use a built in function like max instead of writing our own logic out. It is more concise overall. How do you understand the max() function? How does it work under the hood?
You'll find me asking you about built in functions to ensure that you're clear on what's happening under the hood in case it comes up in an interview.
| if len(current_winner) == 10: | ||
| return (current_winner, highest_key) | ||
| elif len(highest_words_list[index]) == 10: | ||
| return (highest_words_list[index], highest_key) | ||
| elif len(current_winner) > len(highest_words_list[index]): | ||
| current_winner = highest_words_list[index] | ||
| return (current_winner, highest_key) |
There was a problem hiding this comment.
Nice solution here! I see what you're doing by starting the range at 1 which enables us to skip over the current_winner and compare other subsequent tied words if necessary. It is a little unclear to read at first glance and I fired up your code with the debugger to see what was going on.
I wonder if this for loop could be reworked with a for-in loop instead to clarify what's going on.
thank you for feedback :3