Tigers: Sunny and Marie - Adagrams #65
Conversation
… an error saying our tests could not be found.
… uses_available_letters, now tests 1,2, and 3 pass in wave 02
…sing upper method
…except for test 5 and 6
There was a problem hiding this comment.
Great job working together, Sunny and Marie!
Here are a few things to consider for future projects that I've detailed below in your code:
- Some of the functions had for loops that iterated over the same lists twice. That's a great sign to combine the for loops, and maybe turn the conditional statements into compound conditional statements. NOTE: The exception to this rule might be when we are creating frequency maps, or building a data structure that will make iteration easier.
If you have any questions about the feedback, please reach out to me!
adagrams/game.py
Outdated
| @@ -1,11 +1,128 @@ | |||
| from multiprocessing import current_process | |||
There was a problem hiding this comment.
oops looks like VSCode brought in an import line automatically. I don't see this used anywhere in your code, so let's take it out!
| from multiprocessing import current_process |
| for letter in LETTER_POOL: | ||
| for num in range(0,(LETTER_POOL[letter])): | ||
| letter_pool_list.append(letter) |
There was a problem hiding this comment.
Good idea here! This will allow for a better distribution of probability when picking letters randomly.
adagrams/game.py
Outdated
| while len(my_ten_letters) < 10: | ||
| for num in range(0,10): | ||
| random_letter = random.choice(letter_pool_list) | ||
| my_ten_letters.append(random_letter) | ||
| my_string_count = my_ten_letters.count(random_letter) | ||
| word_pool_count = letter_pool_list.count(random_letter) | ||
| if my_string_count > word_pool_count: | ||
| my_ten_letters.remove(random_letter) |
There was a problem hiding this comment.
Hmm we are doing some extra work, only to remove it later. Let's refactor this. We know that we want this to run until our list my_ten_letters has 10 items. From here, we can simply remove the random_letter from our letter_pool_list list. Anything left in the letter_pool_list is an accurate representation of what can still be picked. No need to recount the letters that are left.
while len(my_ten_letters) < 10:
random_letter = random.choice(letter_pool_list)
my_ten_letters.append(random_letter)
letter_pool_list.remove(random_letter)
adagrams/game.py
Outdated
| if letter not in letter_bank: | ||
| letter_results.append("f") | ||
| if "f" in letter_results: | ||
| return False |
There was a problem hiding this comment.
If we hit this if statement on line 68, then we know immediately we should return False. No need to keep track of them in a new data structure. As soon as a letter in the word is not found in our hand, we return False.
I would recommend taking the return True out of the for loop, because if the for loop gets through the entire word, we know all letters are accounted for.
adagrams/game.py
Outdated
| for letter in word: | ||
| count_pool_letter = letter_bank.count(letter) | ||
| count_word_letter = word.count(letter) | ||
| if count_word_letter > count_pool_letter: | ||
| return False | ||
| else: | ||
| return True |
There was a problem hiding this comment.
Nice check! Let's consider combining these two for loops together. We can check that the letter is in the letter_bank and that there are enough letters in the letter_bank to make the word.
adagrams/game.py
Outdated
| if len(word) == 0: | ||
| return word_score |
There was a problem hiding this comment.
nice guard clause! let's put this at the toppy top, though.
| if count_word_letter > count_pool_letter: | ||
| return False | ||
| else: | ||
| return True | ||
|
|
||
| def score_word(word): |
adagrams/game.py
Outdated
| if dict[1] == highest_score_value: | ||
| highest_scores.append(dict) |
There was a problem hiding this comment.
This works! But let's bring in the value as its own variable, so it is easier to understand what dict[1] is.
With the items method, we get access to both the key and value by doing: for key, value in dictionary.items()
So, we could say:
for word, score in score_dict.items():
if score == highest_score_value:
# the rest of your code here
adagrams/game.py
Outdated
| for word, score in highest_scores: | ||
| if len(word) >= 10: | ||
| shortest_word = word | ||
| return (shortest_word, highest_score_value) | ||
| elif len(word) < shortest_word_length: | ||
| shortest_word_length = len(word) | ||
| shortest_word = word | ||
|
|
There was a problem hiding this comment.
Nice job! Could we do this all in one for loop? For example, find the score of a word, check if it's the highest score. Or if it's then equal to the highest score, we check if it's shorter or has a length of 10. If none of those are true, then the loop moves on to the next word.
Food for thought!
adagrams/game.py
Outdated
| 'Y': 2, | ||
| 'Z': 1 | ||
| } | ||
| score_chart ={ |
There was a problem hiding this comment.
Let's make this a constant, too. SCORE_CHART. Also stay consistent with your syntax. Your = should have a space before and after it.
…at improves the descriptive quality of the function
No description provided.