-
Notifications
You must be signed in to change notification settings - Fork 89
Cheetah C18 Viktoriia Zolotova & Vivian Zhu #62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Viktoriia and Vivian (Team V)! Your project has been scored as green.
Nice work! Let me know if you have any questions. 😊
# get letter from the copy dict/pool random.ran() | ||
# draw one letter a time, x 10, for loop (0, 10), | ||
# also update the qty. of the letter in each loop | ||
LETTER_POOL = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job creating constants for the letter pool and letter scores! ✨
'Z': 10 | ||
} | ||
|
||
def copy_dictionary(pool): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice helper function! I'd also recommend checking out the deepcopy()
function. :)
letter_bank_dict = copy_dictionary(LETTER_POOL) | ||
while len(res_letters) < 10: | ||
letter = random.choice(list(letter_bank_dict.keys())) | ||
if letter_bank_dict[letter] > 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach to drawing ten random letters absolutely works, but consider a scenario where the random.choice()
function keeps selecting letters that are already at a frequency of 0. There's no telling how many times this loop may need to run! A small change we can make to guarantee our loop will only run 10 times is removing the letter from the dictionary once its frequency is at 0.
To accomplish this, we could refactor your loop to look something like this:
while len(res_letters) < 10:
letter = random.choice(list(letter_bank_dict.keys()))
res_letters.append(letter)
letter_bank_dict[letter] -= 1
if letter_bank_dict[letter] == 0:
del letter_bank_dict[letter]
word_dict = create_dict(word) | ||
letter_bank_dict = create_dict(letter_bank) | ||
|
||
for letter, quantity in word_dict.items(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job with your uses_available_letters()
function! ✨
(optional) There is a solution for this function that only uses one loop and doesn't require any additional data structures to be created. See if you can optimize your space and time complexity! 🤔😌
""" | ||
data_dict = {} | ||
for element in data: | ||
if element not in data_dict: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend checking out the dictionary setdefault()
method. This method could help clean up this if-else
block by handling situations where the dictionary key doesn't exist.
for letter in word: | ||
if letter in SCORE_POOL.keys(): | ||
total_score += SCORE_POOL[letter] | ||
return total_score |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job with your score_word()
function! ✨
win_name = word | ||
|
||
for word in word_list: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This loop header and the loop header on line 141 are identical. Consider how you can combine these two loops to prevent having to iterate over the same data twice.
#check if len of word != 10, word with fewest len win | ||
#and if words with the same length -> pick the first one in tie_word_dict | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting that you have implemented a for-else
loop here! This structure works here, but for-else
loops are really meant to be used when we're searching for values with loops. They're great for conditionally running code if a value wasn't found. You can read more about them here, but this is a more advanced topic and they really aren't used very often. So, no pressure to learn about them.
For this function, I would recommend removing the else statement and having the rest of the code directly after the for loop.
#check if len of word in tie_word_dict = 10, this word win | ||
for word, length in tie_word_dict.items(): | ||
while length == 10: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this was a typo, but this line should be an if statement rather than a while loop. It still works because even though it's a loop, there is a return
statement inside. So, when we find a word that is of length 10, we enter the loop and then immediately exit the loop and the function.
key_list = list(tie_word_dict.keys()) | ||
val_list = list(tie_word_dict.values()) | ||
win_name = key_list[val_list.index(mini_length)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interesting way to find the word matching the mini_length
! Be careful though because this does require creating two new data structures in memory (key_list
and val_list
) and the .index()
method is an O(n) operation. Consider how you could track the word with the shortest length alongside mini_length
in the for loop on line 164.
No description provided.