Sapphire - Monica Lagdaan #119
Conversation
git push q clear
There was a problem hiding this comment.
Hi Monica! Your code passes all the wave 2 tests and 3/4 of the wave 1 tests. It looks like you still have some more work to do for wave 3 and 4 so I'll have to give you a 🔴 grade.
Please work with your 1:1 on the next steps for finishing this project.
In this PR, you'll see comments from me on how to DRY up your code. For future projects, please make your commits small, frequent, and with detailed commit messages! Detailed commit messages are super helpful for future bug hunting!
Otherwise, keep up the good work✨
| LETTER_POOL = { | ||
| 'A': 9, | ||
| 'B': 2, | ||
| 'C': 2, | ||
| 'D': 4, | ||
| 'E': 12, | ||
| 'F': 2, | ||
| 'G': 3, | ||
| 'H': 2, | ||
| 'I': 9, | ||
| 'J': 1, | ||
| 'K': 1, | ||
| 'L': 4, | ||
| 'M': 2, | ||
| 'N': 6, | ||
| 'O': 8, | ||
| 'P': 2, | ||
| 'Q': 1, | ||
| 'R': 6, | ||
| 'S': 4, | ||
| 'T': 6, | ||
| 'U': 4, | ||
| 'V': 2, | ||
| 'W': 2, | ||
| 'X': 1, | ||
| 'Y': 2, | ||
| 'Z': 1 | ||
| } |
There was a problem hiding this comment.
Nice work using the uppercase naming convention to indicate that this is a constant variable ✨
Since LETTER_POOL is accessed by one function, it would be reasonable to place them inside the functions rather than as a constant. There are tradeoffs, the structures would clutter the function some, but it keeps the data as close as possible to where it's being used, and would mean other functions couldn't access it to accidentally alter the content.
adagrams/game.py
Outdated
| def draw_letters(): | ||
| pass | ||
| used_letters = {} | ||
| letters = [] | ||
| possible_letters = list(LETTER_POOL.keys()) | ||
|
|
||
| while len(letters) <= 9: | ||
| random_int = random.randint(0, 25) | ||
| random_l = possible_letters[random_int] | ||
| if not used_letters.get(random_l) == LETTER_POOL[random_l]: | ||
| if (not used_letters.get(random_l)): | ||
| used_letters[random_l] = 0 | ||
| used_letters[random_l] += 1 | ||
| letters.append(random_l) | ||
|
|
||
| return letters |
There was a problem hiding this comment.
Looks like you've made ✨ Hard mode ✨! Each vowel is supposed to have a higher chance of being added to a hand to make word creation easier. So in other words, (from the README.md):
Since there are 12
Es but only 1Z, it should be 12 times as likely for the user to draw anEas aZ.
Randomly choosing a number between 0-25 to use as an index for selecting a letter causes ALL letters to have an equal chance of being selected into your hand.
The tests do not focus on the statistical accuracy of your code. Instead, the tests check for outcomes that should NOT be possible such as having more than 1 Z in our hand.
Hard Mode is a totally valid way of making this project so don't fret about resubmitting or redoing this (if you have time, you definitely can but it's low-priority compared to the other Learn, PSE, and projects you also need to focus on).
adagrams/game.py
Outdated
| winning_word = |
There was a problem hiding this comment.
This line caused all of the tests to fail, what did you intend to add here?
adagrams/game.py
Outdated
| is_valid = False | ||
|
|
||
| for character in upper_word: | ||
| if character in letter_bank: | ||
| is_valid = True | ||
| else: | ||
| is_valid = False | ||
| return is_valid |
There was a problem hiding this comment.
Flag variables are really useful for while loops but for cases when we're using them to exit a function early like this, we can DRY up our code with the following:
for character in upper_word:
if character not in letter_bank:
return False
adagrams/game.py
Outdated
| if character in letter_bank_dict.keys(): | ||
| letter_bank_dict[character] += 1 | ||
| else: | ||
| letter_bank_dict[character] = 1 | ||
|
|
||
| character_frequency_word = upper_word.count(character) | ||
| character_frequency_dict = letter_bank_dict[character] | ||
|
|
||
| if character_frequency_word <= character_frequency_dict: | ||
| return True | ||
| else: | ||
| if character not in letter_bank or character_frequency_word >= character_frequency_dict: | ||
| return False |
There was a problem hiding this comment.
Did you intend on having some of this code inside of the for loop? The character being assessed here is the last letter in upper_word which means that if the word is 'DOG', the letter G is the only letter being added to the character frequency dict. They may not reflect the same for the rest of the letters within the word.
adagrams/game.py
Outdated
| points_dict = { | ||
| 'A': 1, | ||
| 'B': 3, | ||
| 'C': 3, | ||
| 'D': 2, | ||
| 'E': 1, | ||
| 'F': 4, | ||
| 'G': 2, | ||
| 'H': 4, | ||
| 'I': 1, | ||
| 'J': 8, | ||
| 'K': 5, | ||
| 'L': 1, | ||
| 'M': 3, | ||
| 'N': 1, | ||
| 'O': 1, | ||
| 'P': 3, | ||
| 'Q': 10, | ||
| 'R': 1, | ||
| 'S': 1, | ||
| 'T': 1, | ||
| 'U': 1, | ||
| 'V': 4, | ||
| 'W': 4, | ||
| 'X': 8, | ||
| 'Y': 4, | ||
| 'Z': 10 } |
There was a problem hiding this comment.
Optionally, we could declutter this function by moving this dictionary outside and declaring it as a constant variable.
adagrams/game.py
Outdated
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
1 space is sufficient enough for separating logic within a function.
| for key, value in LETTER_POOL.items(): | ||
| for i in range(value): | ||
| possible_letters.append(key) |
There was a problem hiding this comment.
Nice work creating a list and ensuring it reflects the statistical probability of drawing vowels vs. consonants.
| while len(letters_hand) <= 9: | ||
| random_int = random.randint(0,len(possible_letters)- 1) | ||
| chosen_letter = possible_letters[random_int] | ||
| letters_hand.append(chosen_letter) | ||
| possible_letters.remove(chosen_letter) | ||
| return letters_hand |
| upper_word = word.upper() | ||
| letter_bank_copy = letter_bank.copy() | ||
|
|
||
| for character in upper_word: | ||
| if character not in letter_bank_copy: | ||
| return False | ||
| else: | ||
| letter_bank_copy.remove(character) | ||
| return True |
| upper_word = word.upper() | ||
| total_points = 0 | ||
|
|
||
|
|
||
| for character in upper_word: | ||
| total_points += POINTS_DICT[character] | ||
|
|
||
| if len(upper_word) >= 7: | ||
| total_points += 8 | ||
| else: | ||
| total_points += 0 | ||
| return total_points |
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| #indexing into a tuple | ||
|
|
||
| # if 2 scorez are the same: | ||
| # len of shortest word wins | ||
| # elif len(word) == 10 | ||
| # that word wins | ||
| # elif if len(word) == len(word) and score ==score: | ||
| # pick the first occuring |
There was a problem hiding this comment.
Commented out code should be removed from PR's.
adagrams/game.py
Outdated
| def get_highest_word_score(word_list): | ||
| pass No newline at end of file | ||
| highest_score = 0 | ||
| score_dict = {} | ||
| winning_word = "" | ||
| tie_breaker = {} | ||
|
|
||
| #calculates word & score & adds to score_dict | ||
| for word in word_list: | ||
| scorez = score_word(word) | ||
| score_dict[word] = scorez | ||
|
|
||
| # assigns highest_score & winning_word | ||
| for word, scorez in score_dict.items(): | ||
| if scorez > highest_score: | ||
| highest_score = scorez | ||
| winning_word = word | ||
|
|
||
| elif scorez == highest_score: | ||
| if len(winning_word) == 10: | ||
| pass | ||
| elif len(word) == 10: | ||
| winning_word = word | ||
| elif len(word) < len(winning_word): | ||
| winning_word = word | ||
| elif len(word) == len(winning_word): | ||
| pass | ||
| return (winning_word, highest_score) |
There was a problem hiding this comment.
Instead of pass we could use continue. pass is moreso used as a placeholder keyword for class and function definitions with no function body. continue is more descriptive of moving onto the next iteration if a specific condition is met.
No description provided.