C19 - Amber Shay #112
C19 - Amber Shay #112
Conversation
This reverts commit d1e8b52.
There was a problem hiding this comment.
Nicely done, Amber! Below is some feedback and suggestions. The one pattern I saw was having several nested loops in your functions. This takes practice to catch, but if you do see you've got more than 2 nested loops, consider refactoring!
Also, great job using docstrings to summarize your function's utility!
| letters = [] | ||
|
|
||
| while len(letters) < 10: | ||
| letter_key = random.choice(list(letter_pool_copy.keys())) |
There was a problem hiding this comment.
Nice idea! One thing to consider: should each letter have the same probability of being chosen, though? For example, should "Z" have a probability of 1 in 26 or 1 in 98 (the total amount of tiles that can be drawn)?
How could we represent that in a data structure? Maybe a list of all tiles and their duplicates (ie, ['A','A','A','B','B'] and so on) to randomly choose from.
A more advanced solution might be using the random.choice method, but with more than one param. Here's an example of how you can "weigh" your string or list: https://www.geeksforgeeks.org/how-to-get-weighted-random-choice-in-python/
| letter_bank_case = [element.upper() for element in letter_bank_copy] | ||
|
|
||
| for letter in word.upper(): | ||
| letter.upper() |
There was a problem hiding this comment.
We don't need this a second time. The letter value will be uppercase already because of word.upper()
| letter.upper() |
| letter_bank_copy = letter_bank.copy() | ||
| letter_bank_case = [element.upper() for element in letter_bank_copy] |
There was a problem hiding this comment.
Consider combining these into one! Line 65 is not making a true copy of letter_bank. copy makes a "shallow" copy that still points to the original reference IDs of the items inside the list. Let's refactor line 66 a bit:
letter_bank_copy = [element.upper() for element in letter_bank.deepcopy()]This would now make a "deep" copy of each value in the list, and then convert each value to its uppercase in memory,.
| for letter in word: | ||
| letter = letter.upper() |
There was a problem hiding this comment.
Let's combine these lines:
for letter in word.upper():| letters.append(letter_key) | ||
|
|
||
| return letters | ||
|
|
||
|
|
||
| def uses_available_letters(word, letter_bank): |
There was a problem hiding this comment.
This works fine! We've only just talked about Big O and time/space complexity, but one thing to notice here is we have a 3 nested for loops, even if we haven't explicitly stated for___ in ___ three times.
The remove method loops through letter_bank_case and if letter in letter_bank_case loops under the hood. Nothing wrong with 2 nested for loops. At times they are the easier solution. But! we have 3 here, and a dictionary of letters counted from the letter_bank_case would help us here.
How might we create a "frequency map" (a dictionary of items counted) with a constant lookup (O(1)) of letters found in letter_bank_case, then use it to check if all those letters are in the word?
| return False | ||
|
|
||
| return True | ||
|
|
||
|
|
||
| def score_word(word): |
There was a problem hiding this comment.
Here again we've got 3 nested loops. Let's refactor the letter_value dictionary so that each letter is its own key. Is there some overhead setting that up? Yes. But it would also let us look up a letter's score in "constant" time (letter in score_dict) rather than iterating through an entire list, which is "linear" time (O(n)) (letter in ['A', 'E', 'I', 'O', 'U', 'L', 'N', 'R', 'S', 'T']).
Again, we only just discussed time complexity and Big O, but that's a brief explanation as to why a dictionary is a beneficial data structure we can use to hold data that needs to be frequently looked up.
| for word in word_list: | ||
| score = score_word(word) | ||
| print(f'word: {word}, score: {score}') |
There was a problem hiding this comment.
Don't forget to remove any print statements that are for debugging purposes when you submit a final project!
| print(f'word: {word}, score: {score}') |
| winning_word = "" | ||
|
|
||
| for word in word_list: |
There was a problem hiding this comment.
👍 Your conditional statements are well organized and easy to read! Good job!
No description provided.